diff --git a/CHANGELOG.md b/CHANGELOG.md index f5071899cd5..fc3e0e00c4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Gate Slack startup user allowlist resolution [AI]. (#77898) Thanks @pgondhi987. - OpenAI/Codex: suppress stale `openai-codex` GPT-5.1/5.2/5.3 model refs that ChatGPT/Codex OAuth accounts now reject, keeping model lists, config validation, and forward-compat resolution on current 5.4/5.5 routes. Fixes #67158. Thanks @drpau. - Google Meet/Voice Call: wait longer before playing PIN-derived Twilio DTMF for Meet dial-in prompts and retire stale delegated phone sessions instead of reusing completed calls. - PDF/Codex: include extraction-fallback instructions for `openai-codex/*` PDF tool requests so Codex Responses receives its required system prompt. Fixes #77872. Thanks @anyech. diff --git a/extensions/slack/src/monitor.test-helpers.ts b/extensions/slack/src/monitor.test-helpers.ts index 071e9508ffe..43b5598d3dc 100644 --- a/extensions/slack/src/monitor.test-helpers.ts +++ b/extensions/slack/src/monitor.test-helpers.ts @@ -19,6 +19,9 @@ type SlackTestState = { reactionRemoveMock: Mock<(...args: unknown[]) => unknown>; readAllowFromStoreMock: Mock<(...args: unknown[]) => Promise>; upsertPairingRequestMock: Mock<(...args: unknown[]) => Promise>; + resolveSlackUserAllowlistMock: Mock< + (params: { entries: string[] }) => Promise> + >; }; const slackTestState: SlackTestState = vi.hoisted(() => ({ @@ -31,6 +34,7 @@ const slackTestState: SlackTestState = vi.hoisted(() => ({ reactionRemoveMock: vi.fn(), readAllowFromStoreMock: vi.fn(), upsertPairingRequestMock: vi.fn(), + resolveSlackUserAllowlistMock: vi.fn(), })); export const getSlackTestState = (): SlackTestState => slackTestState; @@ -199,6 +203,11 @@ export function resetSlackTestState(config: Record = defaultSla code: "PAIRCODE", created: true, }); + slackTestState.resolveSlackUserAllowlistMock + .mockReset() + .mockImplementation(async ({ entries }) => + entries.map((input) => ({ input, resolved: false })), + ); getSlackHandlers()?.clear(); } @@ -240,8 +249,8 @@ vi.mock("./resolve-channels.js", () => ({ })); vi.mock("./resolve-users.js", () => ({ - resolveSlackUserAllowlist: async ({ entries }: { entries: string[] }) => - entries.map((input) => ({ input, resolved: false })), + resolveSlackUserAllowlist: (params: { entries: string[] }) => + slackTestState.resolveSlackUserAllowlistMock(params), })); vi.mock("./monitor/send.runtime.js", () => { diff --git a/extensions/slack/src/monitor/provider.allowlist.test.ts b/extensions/slack/src/monitor/provider.allowlist.test.ts index df1e39c9caa..298575db4b7 100644 --- a/extensions/slack/src/monitor/provider.allowlist.test.ts +++ b/extensions/slack/src/monitor/provider.allowlist.test.ts @@ -1,6 +1,21 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it } from "vitest"; +import { + flush, + getSlackHandlerOrThrow, + getSlackTestState, + resetSlackTestState, + startSlackMonitor, + stopSlackMonitor, +} from "../monitor.test-helpers.js"; import { formatSlackChannelResolved, formatSlackUserResolved } from "./provider-support.js"; +const { monitorSlackProvider } = await import("./provider.js"); +const slackTestState = getSlackTestState(); + +beforeEach(() => { + resetSlackTestState(); +}); + describe("slack allowlist log formatting", () => { it("prints channel names alongside ids", () => { expect( @@ -24,3 +39,99 @@ describe("slack allowlist log formatting", () => { ).toBe("U090HHQ029J→steipete (id:U090HHQ029J)"); }); }); + +describe("slack startup user allowlist resolution", () => { + it("skips user entry resolution when name matching is not enabled", async () => { + resetSlackTestState({ + messages: { + responsePrefix: "PFX", + }, + channels: { + slack: { + enabled: true, + dmPolicy: "allowlist", + allowFrom: ["<@U123GLOBAL>", "@global-user"], + channels: { + C123: { + enabled: true, + requireMention: false, + users: ["<@U123CHANNEL>", "@channel-user"], + }, + }, + }, + }, + }); + slackTestState.replyMock.mockResolvedValue({ text: "ok" }); + + const monitor = startSlackMonitor(monitorSlackProvider); + try { + const handler = await getSlackHandlerOrThrow("message"); + await flush(); + await flush(); + + expect(slackTestState.resolveSlackUserAllowlistMock).not.toHaveBeenCalled(); + + await handler({ + event: { + type: "message", + user: "U123GLOBAL", + text: "hello", + ts: "100.000", + channel: "D123", + channel_type: "im", + }, + }); + expect(slackTestState.replyMock).toHaveBeenCalledTimes(1); + + slackTestState.replyMock.mockClear(); + await handler({ + event: { + type: "message", + user: "U123CHANNEL", + text: "hello", + ts: "101.000", + channel: "C123", + channel_type: "channel", + }, + }); + expect(slackTestState.replyMock).toHaveBeenCalledTimes(1); + } finally { + await stopSlackMonitor(monitor); + } + }); + + it("resolves user entries when name matching is enabled", async () => { + resetSlackTestState({ + channels: { + slack: { + enabled: true, + dangerouslyAllowNameMatching: true, + dmPolicy: "allowlist", + allowFrom: ["@global-user"], + channels: { + C123: { users: ["@channel-user"] }, + }, + }, + }, + }); + + const monitor = startSlackMonitor(monitorSlackProvider); + try { + await getSlackHandlerOrThrow("message"); + await flush(); + await flush(); + + expect(slackTestState.resolveSlackUserAllowlistMock).toHaveBeenCalledTimes(2); + expect(slackTestState.resolveSlackUserAllowlistMock).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ entries: ["@global-user"] }), + ); + expect(slackTestState.resolveSlackUserAllowlistMock).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ entries: ["@channel-user"] }), + ); + } finally { + await stopSlackMonitor(monitor); + } + }); +}); diff --git a/extensions/slack/src/monitor/provider.ts b/extensions/slack/src/monitor/provider.ts index 60d84dd2c8f..79198b34b55 100644 --- a/extensions/slack/src/monitor/provider.ts +++ b/extensions/slack/src/monitor/provider.ts @@ -32,7 +32,7 @@ import { isSlackExecApprovalClientEnabled } from "../exec-approvals.js"; import { normalizeSlackWebhookPath, registerSlackHttpHandler } from "../http/index.js"; import { SLACK_TEXT_LIMIT } from "../limits.js"; import { resolveSlackChannelAllowlist } from "../resolve-channels.js"; -import { resolveSlackUserAllowlist } from "../resolve-users.js"; +import { resolveSlackUserAllowlist, type SlackUserResolution } from "../resolve-users.js"; import { resolveSlackAppToken, resolveSlackBotToken } from "../token.js"; import { normalizeAllowList } from "./allow-list.js"; import { resolveSlackSlashCommandConfig } from "./commands.js"; @@ -85,6 +85,33 @@ async function getSlackBoltInterop(): Promise { const SLACK_WEBHOOK_MAX_BODY_BYTES = 1024 * 1024; const SLACK_WEBHOOK_BODY_TIMEOUT_MS = 30_000; +function resolveStableSlackUserIdEntry(raw: string): string | undefined { + const trimmed = raw.trim(); + if (!trimmed) { + return undefined; + } + const mention = /^<@([A-Z][A-Z0-9]+)>$/i.exec(trimmed); + if (mention) { + return mention[1]?.toUpperCase(); + } + const prefixed = /^(?:slack:|user:)([A-Z][A-Z0-9]+)$/i.exec(trimmed); + if (prefixed) { + return prefixed[1]?.toUpperCase(); + } + return /^[UW][A-Z0-9]+$/i.test(trimmed) ? trimmed.toUpperCase() : undefined; +} + +function resolveStableSlackUserAllowlistEntries(entries: string[]): SlackUserResolution[] { + const resolved: SlackUserResolution[] = []; + for (const input of entries) { + const id = resolveStableSlackUserIdEntry(input); + if (id) { + resolved.push({ input, resolved: true, id }); + } + } + return resolved; +} + export function formatSlackSocketReconnectMessage(params: { event: string; attempt: number; @@ -209,6 +236,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { const threadInheritParent = slackCfg.thread?.inheritParent ?? false; const threadRequireExplicitMention = slackCfg.thread?.requireExplicitMention ?? false; const slashCommand = resolveSlackSlashCommandConfig(opts.slashCommand ?? slackCfg.slashCommand); + const allowNameMatching = isDangerousNameMatchingEnabled(slackCfg); const textLimit = resolveTextChunkLimit(cfg, "slack", account.accountId, { fallbackLimit: SLACK_TEXT_LIMIT, }); @@ -301,7 +329,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { dmEnabled, dmPolicy, allowFrom, - allowNameMatching: isDangerousNameMatchingEnabled(slackCfg), + allowNameMatching, groupDmEnabled, groupDmChannels, defaultRequireMention: slackCfg.requireMention, @@ -404,24 +432,36 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { const allowEntries = normalizeStringEntries(allowFrom).filter((entry) => entry !== "*"); if (allowEntries.length > 0) { - try { - const resolvedUsers = await resolveSlackUserAllowlist({ - token: resolveToken, - entries: allowEntries, + const stableResolvedUsers = resolveStableSlackUserAllowlistEntries(allowEntries); + if (stableResolvedUsers.length > 0) { + const { mapping, additions } = buildAllowlistResolutionSummary(stableResolvedUsers, { + formatResolved: formatSlackUserResolved, }); - const { mapping, unresolved, additions } = buildAllowlistResolutionSummary( - resolvedUsers, - { - formatResolved: formatSlackUserResolved, - }, - ); allowFrom = mergeAllowlist({ existing: allowFrom, additions }); ctx.allowFrom = normalizeAllowList(allowFrom); - summarizeMapping("slack users", mapping, unresolved, runtime); - } catch (err) { - runtime.log?.( - `slack user resolve failed; using config entries. ${formatUnknownError(err)}`, - ); + summarizeMapping("slack users", mapping, [], runtime); + } + + if (allowNameMatching) { + try { + const resolvedUsers = await resolveSlackUserAllowlist({ + token: resolveToken, + entries: allowEntries, + }); + const { mapping, unresolved, additions } = buildAllowlistResolutionSummary( + resolvedUsers, + { + formatResolved: formatSlackUserResolved, + }, + ); + allowFrom = mergeAllowlist({ existing: allowFrom, additions }); + ctx.allowFrom = normalizeAllowList(allowFrom); + summarizeMapping("slack users", mapping, unresolved, runtime); + } catch (err) { + runtime.log?.( + `slack user resolve failed; using config entries. ${formatUnknownError(err)}`, + ); + } } } @@ -432,29 +472,47 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { } if (userEntries.size > 0) { - try { - const resolvedUsers = await resolveSlackUserAllowlist({ - token: resolveToken, - entries: Array.from(userEntries), + const stableResolvedUsers = resolveStableSlackUserAllowlistEntries( + Array.from(userEntries), + ); + if (stableResolvedUsers.length > 0) { + const { resolvedMap, mapping } = buildAllowlistResolutionSummary(stableResolvedUsers, { + formatResolved: formatSlackUserResolved, }); - const { resolvedMap, mapping, unresolved } = buildAllowlistResolutionSummary( - resolvedUsers, - { - formatResolved: formatSlackUserResolved, - }, - ); - const nextChannels = patchAllowlistUsersInConfigEntries({ entries: channelsConfig, resolvedMap, }); channelsConfig = nextChannels; ctx.channelsConfig = nextChannels; - summarizeMapping("slack channel users", mapping, unresolved, runtime); - } catch (err) { - runtime.log?.( - `slack channel user resolve failed; using config entries. ${formatUnknownError(err)}`, - ); + summarizeMapping("slack channel users", mapping, [], runtime); + } + + if (allowNameMatching) { + try { + const resolvedUsers = await resolveSlackUserAllowlist({ + token: resolveToken, + entries: Array.from(userEntries), + }); + const { resolvedMap, mapping, unresolved } = buildAllowlistResolutionSummary( + resolvedUsers, + { + formatResolved: formatSlackUserResolved, + }, + ); + + const nextChannels = patchAllowlistUsersInConfigEntries({ + entries: channelsConfig, + resolvedMap, + }); + channelsConfig = nextChannels; + ctx.channelsConfig = nextChannels; + summarizeMapping("slack channel users", mapping, unresolved, runtime); + } catch (err) { + runtime.log?.( + `slack channel user resolve failed; using config entries. ${formatUnknownError(err)}`, + ); + } } } }