From 6a60d47c53965960fc260c8611127f3bd1d83a9a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 24 Jan 2026 07:05:31 +0000 Subject: [PATCH] fix: cover slack open policy gating (#1563) (thanks @itsjaydesu) --- CHANGELOG.md | 1 + src/agents/anthropic-payload-log.ts | 21 ++- src/plugins/commands.ts | 8 +- src/slack/monitor/slash.policy.test.ts | 186 +++++++++++++++++++++++++ 4 files changed, 211 insertions(+), 5 deletions(-) create mode 100644 src/slack/monitor/slash.policy.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e4e5ba85..04a687300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Docs: https://docs.clawd.bot - Channels: allow per-group tool allow/deny policies across built-in + plugin channels. (#1546) Thanks @adam91holt. ### Fixes +- Slack: honor open groupPolicy for unlisted channels in message + slash gating. (#1563) Thanks @itsjaydesu. - Agents: ignore IDENTITY.md template placeholders when parsing identity to avoid placeholder replies. (#1556) - Docker: update gateway command in docker-compose and Hetzner guide. (#1514) - Sessions: reject array-backed session stores to prevent silent wipes. (#1469) diff --git a/src/agents/anthropic-payload-log.ts b/src/agents/anthropic-payload-log.ts index 8b39d2f1a..1023f9f45 100644 --- a/src/agents/anthropic-payload-log.ts +++ b/src/agents/anthropic-payload-log.ts @@ -90,6 +90,18 @@ function safeJsonStringify(value: unknown): string | null { } } +function formatError(error: unknown): string | undefined { + if (error instanceof Error) return error.message; + if (typeof error === "string") return error; + if (typeof error === "number" || typeof error === "boolean" || typeof error === "bigint") { + return String(error); + } + if (error && typeof error === "object") { + return safeJsonStringify(error) ?? "unknown error"; + } + return undefined; +} + function digest(value: unknown): string | undefined { const serialized = safeJsonStringify(value); if (!serialized) return undefined; @@ -163,7 +175,7 @@ export function createAnthropicPayloadLogger(params: { options?.onPayload?.(payload); }; return streamFn(model, context, { - ...(options ?? {}), + ...options, onPayload: nextOnPayload, }); }; @@ -172,13 +184,14 @@ export function createAnthropicPayloadLogger(params: { const recordUsage: AnthropicPayloadLogger["recordUsage"] = (messages, error) => { const usage = findLastAssistantUsage(messages); + const errorMessage = formatError(error); if (!usage) { - if (error) { + if (errorMessage) { record({ ...base, ts: new Date().toISOString(), stage: "usage", - error: String(error), + error: errorMessage, }); } return; @@ -188,7 +201,7 @@ export function createAnthropicPayloadLogger(params: { ts: new Date().toISOString(), stage: "usage", usage, - error: error ? String(error) : undefined, + error: errorMessage, }); log.info("anthropic usage", { runId: params.runId, diff --git a/src/plugins/commands.ts b/src/plugins/commands.ts index 68f232b39..27e424303 100644 --- a/src/plugins/commands.ts +++ b/src/plugins/commands.ts @@ -195,7 +195,13 @@ function sanitizeArgs(args: string | undefined): string | undefined { } // Remove control characters (except newlines and tabs which may be intentional) - return args.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, ""); + let sanitized = ""; + for (const char of args) { + const code = char.charCodeAt(0); + const isControl = (code <= 0x1f && code !== 0x09 && code !== 0x0a) || code === 0x7f; + if (!isControl) sanitized += char; + } + return sanitized; } /** diff --git a/src/slack/monitor/slash.policy.test.ts b/src/slack/monitor/slash.policy.test.ts new file mode 100644 index 000000000..182e7a015 --- /dev/null +++ b/src/slack/monitor/slash.policy.test.ts @@ -0,0 +1,186 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { registerSlackMonitorSlashCommands } from "./slash.js"; + +const dispatchMock = vi.fn(); +const readAllowFromStoreMock = vi.fn(); +const upsertPairingRequestMock = vi.fn(); +const resolveAgentRouteMock = vi.fn(); + +vi.mock("../../auto-reply/reply/provider-dispatcher.js", () => ({ + dispatchReplyWithDispatcher: (...args: unknown[]) => dispatchMock(...args), +})); + +vi.mock("../../pairing/pairing-store.js", () => ({ + readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args), + upsertChannelPairingRequest: (...args: unknown[]) => upsertPairingRequestMock(...args), +})); + +vi.mock("../../routing/resolve-route.js", () => ({ + resolveAgentRoute: (...args: unknown[]) => resolveAgentRouteMock(...args), +})); + +vi.mock("../../agents/identity.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + resolveEffectiveMessagesConfig: () => ({ responsePrefix: "" }), + }; +}); + +function createHarness(overrides?: { + groupPolicy?: "open" | "allowlist"; + channelsConfig?: Record; + channelId?: string; + channelName?: string; +}) { + const commands = new Map Promise>(); + const postEphemeral = vi.fn().mockResolvedValue({ ok: true }); + const app = { + client: { chat: { postEphemeral } }, + command: (name: unknown, handler: (args: unknown) => Promise) => { + commands.set(name, handler); + }, + }; + + const channelId = overrides?.channelId ?? "C_UNLISTED"; + const channelName = overrides?.channelName ?? "unlisted"; + + const ctx = { + cfg: { commands: { native: false } }, + runtime: {}, + botToken: "bot-token", + botUserId: "bot", + teamId: "T1", + allowFrom: ["*"], + dmEnabled: true, + dmPolicy: "open", + groupDmEnabled: false, + groupDmChannels: [], + defaultRequireMention: true, + groupPolicy: overrides?.groupPolicy ?? "open", + useAccessGroups: true, + channelsConfig: overrides?.channelsConfig, + slashCommand: { enabled: true, name: "clawd", ephemeral: true, sessionPrefix: "slack:slash" }, + textLimit: 4000, + app, + isChannelAllowed: () => true, + resolveChannelName: async () => ({ name: channelName, type: "channel" }), + resolveUserName: async () => ({ name: "Ada" }), + } as unknown; + + const account = { accountId: "acct", config: { commands: { native: false } } } as unknown; + + return { commands, ctx, account, postEphemeral, channelId, channelName }; +} + +beforeEach(() => { + dispatchMock.mockReset().mockResolvedValue({ counts: { final: 1, tool: 0, block: 0 } }); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + upsertPairingRequestMock.mockReset().mockResolvedValue({ code: "PAIRCODE", created: true }); + resolveAgentRouteMock.mockReset().mockReturnValue({ + agentId: "main", + sessionKey: "session:1", + accountId: "acct", + }); +}); + +describe("slack slash commands channel policy", () => { + it("allows unlisted channels when groupPolicy is open", async () => { + const { commands, ctx, account, channelId, channelName } = createHarness({ + groupPolicy: "open", + channelsConfig: { C_LISTED: { requireMention: true } }, + channelId: "C_UNLISTED", + channelName: "unlisted", + }); + registerSlackMonitorSlashCommands({ ctx: ctx as never, account: account as never }); + + const handler = [...commands.values()][0]; + if (!handler) throw new Error("Missing slash handler"); + + const respond = vi.fn().mockResolvedValue(undefined); + await handler({ + command: { + user_id: "U1", + user_name: "Ada", + channel_id: channelId, + channel_name: channelName, + text: "hello", + trigger_id: "t1", + }, + ack: vi.fn().mockResolvedValue(undefined), + respond, + }); + + expect(dispatchMock).toHaveBeenCalledTimes(1); + expect(respond).not.toHaveBeenCalledWith( + expect.objectContaining({ text: "This channel is not allowed." }), + ); + }); + + it("blocks explicitly denied channels when groupPolicy is open", async () => { + const { commands, ctx, account, channelId, channelName } = createHarness({ + groupPolicy: "open", + channelsConfig: { C_DENIED: { allow: false } }, + channelId: "C_DENIED", + channelName: "denied", + }); + registerSlackMonitorSlashCommands({ ctx: ctx as never, account: account as never }); + + const handler = [...commands.values()][0]; + if (!handler) throw new Error("Missing slash handler"); + + const respond = vi.fn().mockResolvedValue(undefined); + await handler({ + command: { + user_id: "U1", + user_name: "Ada", + channel_id: channelId, + channel_name: channelName, + text: "hello", + trigger_id: "t1", + }, + ack: vi.fn().mockResolvedValue(undefined), + respond, + }); + + expect(dispatchMock).not.toHaveBeenCalled(); + expect(respond).toHaveBeenCalledWith({ + text: "This channel is not allowed.", + response_type: "ephemeral", + }); + }); + + it("blocks unlisted channels when groupPolicy is allowlist", async () => { + const { commands, ctx, account, channelId, channelName } = createHarness({ + groupPolicy: "allowlist", + channelsConfig: { C_LISTED: { requireMention: true } }, + channelId: "C_UNLISTED", + channelName: "unlisted", + }); + registerSlackMonitorSlashCommands({ ctx: ctx as never, account: account as never }); + + const handler = [...commands.values()][0]; + if (!handler) throw new Error("Missing slash handler"); + + const respond = vi.fn().mockResolvedValue(undefined); + await handler({ + command: { + user_id: "U1", + user_name: "Ada", + channel_id: channelId, + channel_name: channelName, + text: "hello", + trigger_id: "t1", + }, + ack: vi.fn().mockResolvedValue(undefined), + respond, + }); + + expect(dispatchMock).not.toHaveBeenCalled(); + expect(respond).toHaveBeenCalledWith({ + text: "This channel is not allowed.", + response_type: "ephemeral", + }); + }); +});