mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-07 02:22:46 +00:00
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 <li.long15@xydigit.com>
This commit is contained in:
Alix-007
@@ -2,6 +2,19 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { slackDoctor } from "./doctor.js";
|
||||
|
||||
async function collectSlackWarnings(
|
||||
slack: Record<string, unknown>,
|
||||
defaults?: Record<string, unknown>,
|
||||
) {
|
||||
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();
|
||||
|
||||
|
||||
@@ -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<string>();
|
||||
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 }),
|
||||
],
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user