From 26281a8a11190d7c337cefb229cafa896528ffa4 Mon Sep 17 00:00:00 2001 From: Alix-007 Date: Sat, 13 Jun 2026 18:13:14 +0800 Subject: [PATCH] fix(slack): diagnose invalid channel map keys (#89438) Diagnose Slack channel-map keys that cannot route as configured, including account inheritance, open-policy overrides, malformed room identifiers, and DM identifiers. Fixes #81665 Co-authored-by: Alix-007 --- extensions/slack/src/doctor.test.ts | 243 ++++++++++++++++++++++++++++ extensions/slack/src/doctor.ts | 135 +++++++++++++++- 2 files changed, 377 insertions(+), 1 deletion(-) diff --git a/extensions/slack/src/doctor.test.ts b/extensions/slack/src/doctor.test.ts index 8ddfd947e37b..3dfab36ab1ae 100644 --- a/extensions/slack/src/doctor.test.ts +++ b/extensions/slack/src/doctor.test.ts @@ -2,6 +2,19 @@ import { describe, expect, it } from "vitest"; import { slackDoctor } from "./doctor.js"; +async function collectSlackWarnings( + slack: Record, + defaults?: Record, +) { + return ( + (await Promise.resolve( + slackDoctor.collectMutableAllowlistWarnings?.({ + cfg: { channels: { ...(defaults ? { defaults } : {}), slack } } as never, + }), + )) ?? [] + ); +} + function getSlackCompatibilityNormalizer(): NonNullable< typeof slackDoctor.normalizeCompatibilityConfig > { @@ -50,6 +63,236 @@ describe("slack doctor", () => { ).toBe(true); }); + it("warns for name-keyed allowlist channels but accepts routed ID forms (#81665)", async () => { + const warnings = await collectSlackWarnings({ + channels: { + "example-channel": {}, + community: {}, + C0AL2GDUA7J: {}, + c0al2gdua7k: {}, + "channel:C0AL2GDUA7L": {}, + "channel:c0al2gdua7m": {}, + D0AL2GDUA7Q: {}, + "channel:d0al2gdua7r": {}, + "channel:dabcdefgh": {}, + "channel:customers": {}, + "CHANNEL:C0AL2GDUA7N": {}, + "channel:C0al2gdua7p": {}, + "*": {}, + }, + }); + + const nameKeyWarnings = warnings.filter((warning) => + warning.includes("Re-key it with the channel's"), + ); + expect(nameKeyWarnings).toHaveLength(5); + expect(nameKeyWarnings[0]).toContain('channels.slack.channels."example-channel"'); + expect(nameKeyWarnings[0]).toContain('channels.slack.channels."*" applies instead'); + expect(nameKeyWarnings[1]).toContain('channels.slack.channels."community" is ambiguous'); + expect(nameKeyWarnings[2]).toContain( + 'channels.slack.channels."channel:customers" is ambiguous', + ); + expect(nameKeyWarnings[3]).toContain('channels.slack.channels."CHANNEL:C0AL2GDUA7N"'); + expect(nameKeyWarnings[4]).toContain('channels.slack.channels."channel:C0al2gdua7p"'); + const dmWarnings = warnings.filter((warning) => + warning.includes("is a Slack DM conversation ID"), + ); + expect(dmWarnings).toHaveLength(3); + expect(dmWarnings[0]).toContain('channels.slack.channels."D0AL2GDUA7Q"'); + expect(dmWarnings[1]).toContain('channels.slack.channels."channel:d0al2gdua7r"'); + expect(dmWarnings[2]).toContain('channels.slack.channels."channel:dabcdefgh"'); + expect(dmWarnings[0]).toContain("channels.slack.dmPolicy"); + }); + + it("uses account policy and name-matching overrides for name-keyed channels (#81665)", async () => { + const overlongName = "a".repeat(81); + const warnings = await collectSlackWarnings({ + groupPolicy: "open", + channels: { "root-room": {} }, + accounts: { + inheritedOpen: { + channels: { general: {} }, + }, + inheritedAllowlist: { + groupPolicy: "allowlist", + }, + explicitAllowlist: { + groupPolicy: "allowlist", + channels: { engineering: {} }, + }, + nameMatching: { + groupPolicy: "allowlist", + dangerouslyAllowNameMatching: true, + channels: { + support: {}, + "#help": {}, + "crème-brûlée": {}, + d0customers: {}, + dabcdefgh: {}, + "channel:customers": {}, + "<#C0AL2GDUA7J>": {}, + "slack:C0AL2GDUA7K": {}, + "@help": {}, + "##help": {}, + "help+": {}, + Support: {}, + "-": {}, + ___: {}, + "#--": {}, + [overlongName]: {}, + }, + }, + }, + }); + + const nameKeyWarnings = warnings.filter((warning) => + warning.includes("Re-key it with the channel's"), + ); + expect(nameKeyWarnings).toHaveLength(13); + const rootWarning = nameKeyWarnings.find((warning) => + warning.includes('channels.slack.channels."root-room"'), + ); + expect(rootWarning).toContain("messages from the channel are dropped"); + expect( + nameKeyWarnings.some((warning) => + warning.includes('channels.slack.accounts.explicitAllowlist.channels."engineering"'), + ), + ).toBe(true); + expect( + nameKeyWarnings.some((warning) => + warning.includes( + 'channels.slack.accounts.nameMatching.channels."channel:customers" is ambiguous', + ), + ), + ).toBe(true); + expect( + nameKeyWarnings.some((warning) => + warning.includes('channels.slack.accounts.nameMatching.channels."<#C0AL2GDUA7J>"'), + ), + ).toBe(true); + expect( + nameKeyWarnings.some((warning) => + warning.includes('channels.slack.accounts.nameMatching.channels."slack:C0AL2GDUA7K"'), + ), + ).toBe(true); + for (const invalidName of [ + "@help", + "##help", + "help+", + "Support", + "-", + "___", + "#--", + overlongName, + ]) { + expect( + nameKeyWarnings.some((warning) => + warning.includes(`channels.slack.accounts.nameMatching.channels."${invalidName}"`), + ), + ).toBe(true); + } + + const sharedOpenWarnings = await collectSlackWarnings( + { channels: { "shared-room": {} } }, + { groupPolicy: "open" }, + ); + expect( + sharedOpenWarnings.some((warning) => warning.includes("not a routable Slack channel ID")), + ).toBe(true); + }); + + it("warns when an open-policy override is keyed by channel name (#81665)", async () => { + const warnings = await collectSlackWarnings({ + groupPolicy: "open", + channels: { + "private-room": { enabled: false }, + }, + }); + + expect(warnings).toEqual([expect.stringContaining('channels.slack.channels."private-room"')]); + expect(warnings[0]).toContain("the channel remains allowed"); + }); + + it("warns for DM IDs regardless of room policy and uses account-scoped remediation", async () => { + const openWarnings = await collectSlackWarnings({ + groupPolicy: "open", + channels: { + D0AL2GDUA7S: {}, + }, + }); + expect(openWarnings).toEqual([ + expect.stringContaining('channels.slack.channels."D0AL2GDUA7S"'), + ]); + + const disabledAccountWarnings = await collectSlackWarnings({ + accounts: { + work: { + groupPolicy: "disabled", + channels: { + "channel:d0al2gdua7t": {}, + }, + }, + }, + }); + expect(disabledAccountWarnings).toEqual([ + expect.stringContaining('channels.slack.accounts.work.channels."channel:d0al2gdua7t"'), + ]); + expect(disabledAccountWarnings[0]).toContain("channels.slack.accounts.work.dmPolicy"); + expect(disabledAccountWarnings[0]).toContain("channels.slack.accounts.work.allowFrom"); + + const inheritedChannelWarnings = await collectSlackWarnings({ + channels: { + D0AL2GDUA7U: {}, + }, + accounts: { + work: { + groupPolicy: "disabled", + dmPolicy: "allowlist", + allowFrom: ["U0AL2GDUA7U"], + }, + }, + }); + expect(inheritedChannelWarnings).toEqual([ + expect.stringContaining('channels.slack.channels."D0AL2GDUA7U"'), + ]); + expect(inheritedChannelWarnings[0]).toContain("channels.slack.accounts.work.dmPolicy"); + }); + + it("treats bare lowercase D forms as ambiguous without name matching", async () => { + const warnings = await collectSlackWarnings({ + channels: { + d0customers: {}, + dabcdefgh: {}, + }, + }); + + expect(warnings).toHaveLength(2); + expect(warnings[0]).toContain( + 'channels.slack.channels."d0customers" is ambiguous: it may be a lowercase Slack DM conversation ID or a channel name', + ); + expect(warnings[1]).toContain( + 'channels.slack.channels."dabcdefgh" is ambiguous: it may be a lowercase Slack DM conversation ID or a channel name', + ); + expect(warnings[0]).toContain("stable C/G ID"); + }); + + it("does not audit provider defaults as a standalone named account (#81665)", async () => { + const warnings = await collectSlackWarnings({ + channels: { + "provider-room": { enabled: false }, + }, + accounts: { + work: { + channels: { + C0AL2GDUA7J: {}, + }, + }, + }, + }); + + expect(warnings.some((warning) => warning.includes("provider-room"))).toBe(false); + }); + it("normalizes legacy slack streaming aliases into the nested streaming shape", () => { const normalize = getSlackCompatibilityNormalizer(); diff --git a/extensions/slack/src/doctor.ts b/extensions/slack/src/doctor.ts index 2633fb55cb3e..e4433c9a3c52 100644 --- a/extensions/slack/src/doctor.ts +++ b/extensions/slack/src/doctor.ts @@ -1,6 +1,8 @@ // Slack plugin module implements doctor behavior. import type { ChannelDoctorAdapter } from "openclaw/plugin-sdk/channel-contract"; import { createDangerousNameMatchingMutableAllowlistWarningCollector } from "openclaw/plugin-sdk/channel-policy"; +import type { GroupPolicy, OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; +import { listSlackAccountIds, mergeSlackAccountConfig } from "./accounts.js"; import { legacyConfigRules as SLACK_LEGACY_CONFIG_RULES, normalizeCompatibilityConfig as normalizeSlackCompatibilityConfig, @@ -48,6 +50,134 @@ const collectSlackMutableAllowlistWarnings = }, }); +const SLACK_CANONICAL_CHANNEL_ID_RE = /^[CG][A-Z0-9]{8,}$/; +const SLACK_LOWERCASE_CHANNEL_ID_RE = /^[cg][0-9][a-z0-9]{7,}$/; +const SLACK_PREFIXED_CANONICAL_CHANNEL_ID_RE = /^channel:[CG][A-Z0-9]{8,}$/; +const SLACK_PREFIXED_LOWERCASE_CHANNEL_ID_RE = /^channel:[cg][0-9][a-z0-9]{7,}$/; +const SLACK_CANONICAL_DM_ID_RE = /^(?:channel:)?D[A-Z0-9]{8,}$/; +const SLACK_PREFIXED_LOWERCASE_DM_ID_RE = /^channel:d[a-z0-9]{8,}$/; +const SLACK_AMBIGUOUS_LOWERCASE_DM_ID_RE = /^d[a-z0-9]{8,}$/; +// Letter-leading lowercase forms may be valid IDs or human names. Warn conditionally instead of +// claiming they are unroutable. +const SLACK_AMBIGUOUS_LOWERCASE_CHANNEL_ID_RE = /^(?:channel:)?[cgd][a-z][a-z0-9]{7,}$/; +// Slack supports international channel names, and runtime name matching preserves exact names. +// Keep Unicode letters/marks/numbers while enforcing lowercase, length, and punctuation rules. +const SLACK_CHANNEL_NAME_RE = /^[\p{L}\p{M}\p{N}_-]{1,80}$/u; +const SLACK_CHANNEL_NAME_ALPHANUMERIC_RE = /[\p{L}\p{N}]/u; + +function looksLikeSlackChannelId(channelKey: string): boolean { + return ( + SLACK_CANONICAL_CHANNEL_ID_RE.test(channelKey) || + SLACK_LOWERCASE_CHANNEL_ID_RE.test(channelKey) || + SLACK_PREFIXED_CANONICAL_CHANNEL_ID_RE.test(channelKey) || + SLACK_PREFIXED_LOWERCASE_CHANNEL_ID_RE.test(channelKey) + ); +} + +function looksLikeSlackDmId(channelKey: string): boolean { + return ( + SLACK_CANONICAL_DM_ID_RE.test(channelKey) || SLACK_PREFIXED_LOWERCASE_DM_ID_RE.test(channelKey) + ); +} + +function looksLikeSlackChannelNameKey(channelKey: string): boolean { + const name = channelKey.startsWith("#") ? channelKey.slice(1) : channelKey; + return ( + name === name.toLowerCase() && + SLACK_CHANNEL_NAME_RE.test(name) && + SLACK_CHANNEL_NAME_ALPHANUMERIC_RE.test(name) + ); +} + +// Startup resolution updates ctx.channelsConfig, but inbound authorization captures the authored +// channels map and key list when createSlackMonitorContext runs. Diagnose those authored keys. +function collectSlackNameKeyedChannelWarnings({ cfg }: { cfg: OpenClawConfig }): string[] { + const warnings = new Set(); + const slackCfg = asObjectRecord(asObjectRecord(cfg.channels)?.slack); + const providerChannels = asObjectRecord(slackCfg?.channels); + const accounts = asObjectRecord(slackCfg?.accounts); + for (const accountId of listSlackAccountIds(cfg)) { + const account = asObjectRecord(mergeSlackAccountConfig(cfg, accountId)); + if (!account || slackCfg?.enabled === false || account.enabled === false) { + continue; + } + const scopedGroupPolicy = + typeof account.groupPolicy === "string" ? (account.groupPolicy as GroupPolicy) : undefined; + // Slack's schema materializes this provider default before runtime account merging. + const effectiveGroupPolicy = scopedGroupPolicy ?? "allowlist"; + const rawAccount = asObjectRecord(accounts?.[accountId]); + const accountPrefix = rawAccount ? `channels.slack.accounts.${accountId}` : "channels.slack"; + const accountChannels = asObjectRecord(rawAccount?.channels); + const channels = accountChannels ?? providerChannels; + if (!channels) { + continue; + } + const channelsPrefix = accountChannels + ? `channels.slack.accounts.${accountId}` + : "channels.slack"; + const fallbackDescription = Object.hasOwn(channels, "*") + ? `${channelsPrefix}.channels."*" applies instead and this entry's overrides are ignored` + : effectiveGroupPolicy === "open" + ? 'this entry\'s overrides are ignored and the channel remains allowed by groupPolicy: "open"' + : "messages from the channel are dropped"; + for (const channelKey of Object.keys(channels)) { + if (channelKey === "*") { + continue; + } + if (looksLikeSlackDmId(channelKey)) { + warnings.add( + `${channelsPrefix}.channels."${channelKey}" is a Slack DM conversation ID, but ${channelsPrefix}.channels only configures channel and group rooms. ` + + `Configure DM access with ${accountPrefix}.dmPolicy and ${accountPrefix}.allowFrom instead.`, + ); + continue; + } + if (SLACK_AMBIGUOUS_LOWERCASE_DM_ID_RE.test(channelKey)) { + if ( + account.dangerouslyAllowNameMatching === true && + looksLikeSlackChannelNameKey(channelKey) + ) { + continue; + } + warnings.add( + `${channelsPrefix}.channels."${channelKey}" is ambiguous: it may be a lowercase Slack DM conversation ID or a channel name. ` + + `Configure DMs with ${accountPrefix}.dmPolicy and ${accountPrefix}.allowFrom; otherwise re-key the room with its stable C/G ID.`, + ); + continue; + } + if (effectiveGroupPolicy === "disabled") { + continue; + } + const channelConfig = asObjectRecord(channels[channelKey]); + if (effectiveGroupPolicy === "open" && Object.keys(channelConfig ?? {}).length === 0) { + continue; + } + if (looksLikeSlackChannelId(channelKey)) { + continue; + } + if ( + account.dangerouslyAllowNameMatching === true && + looksLikeSlackChannelNameKey(channelKey) + ) { + continue; + } + if (SLACK_AMBIGUOUS_LOWERCASE_CHANNEL_ID_RE.test(channelKey)) { + warnings.add( + `${channelsPrefix}.channels."${channelKey}" is ambiguous: it may be a lowercase Slack channel ID or a channel name. ` + + `If it is a channel name, inbound routing will not match it and ${fallbackDescription}. ` + + `Re-key it with the channel's stable ID (e.g. C0123ABCD, from the channel's About details or conversations.info).`, + ); + continue; + } + warnings.add( + `${channelsPrefix}.channels."${channelKey}" is keyed by a channel name or non-canonical ID form, not a routable Slack channel ID; ` + + `under groupPolicy: "${effectiveGroupPolicy}" inbound routing does not match this entry, so ${fallbackDescription}. ` + + `Re-key it with the channel's ID (e.g. C0123ABCD, from the channel's About details or conversations.info).`, + ); + } + } + return [...warnings]; +} + export const slackDoctor: ChannelDoctorAdapter = { dmAllowFromMode: "topOnly", groupModel: "route", @@ -55,5 +185,8 @@ export const slackDoctor: ChannelDoctorAdapter = { warnOnEmptyGroupSenderAllowlist: false, legacyConfigRules: SLACK_LEGACY_CONFIG_RULES, normalizeCompatibilityConfig: normalizeSlackCompatibilityConfig, - collectMutableAllowlistWarnings: collectSlackMutableAllowlistWarnings, + collectMutableAllowlistWarnings: ({ cfg }) => [ + ...collectSlackMutableAllowlistWarnings({ cfg }), + ...collectSlackNameKeyedChannelWarnings({ cfg }), + ], };