Skip to content

Commit 26281a8

Browse files
Alix-007Alix-007
authored
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 <[email protected]>
1 parent 4208c89 commit 26281a8

2 files changed

Lines changed: 377 additions & 1 deletion

File tree

‎extensions/slack/src/doctor.test.ts‎

Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,19 @@
22
import { describe, expect, it } from "vitest";
33
import { slackDoctor } from "./doctor.js";
44

5+
async function collectSlackWarnings(
6+
slack: Record<string, unknown>,
7+
defaults?: Record<string, unknown>,
8+
) {
9+
return (
10+
(await Promise.resolve(
11+
slackDoctor.collectMutableAllowlistWarnings?.({
12+
cfg: { channels: { ...(defaults ? { defaults } : {}), slack } } as never,
13+
}),
14+
)) ?? []
15+
);
16+
}
17+
518
function getSlackCompatibilityNormalizer(): NonNullable<
619
typeof slackDoctor.normalizeCompatibilityConfig
720
> {
@@ -50,6 +63,236 @@ describe("slack doctor", () => {
5063
).toBe(true);
5164
});
5265

66+
it("warns for name-keyed allowlist channels but accepts routed ID forms (#81665)", async () => {
67+
const warnings = await collectSlackWarnings({
68+
channels: {
69+
"example-channel": {},
70+
community: {},
71+
C0AL2GDUA7J: {},
72+
c0al2gdua7k: {},
73+
"channel:C0AL2GDUA7L": {},
74+
"channel:c0al2gdua7m": {},
75+
D0AL2GDUA7Q: {},
76+
"channel:d0al2gdua7r": {},
77+
"channel:dabcdefgh": {},
78+
"channel:customers": {},
79+
"CHANNEL:C0AL2GDUA7N": {},
80+
"channel:C0al2gdua7p": {},
81+
"*": {},
82+
},
83+
});
84+
85+
const nameKeyWarnings = warnings.filter((warning) =>
86+
warning.includes("Re-key it with the channel's"),
87+
);
88+
expect(nameKeyWarnings).toHaveLength(5);
89+
expect(nameKeyWarnings[0]).toContain('channels.slack.channels."example-channel"');
90+
expect(nameKeyWarnings[0]).toContain('channels.slack.channels."*" applies instead');
91+
expect(nameKeyWarnings[1]).toContain('channels.slack.channels."community" is ambiguous');
92+
expect(nameKeyWarnings[2]).toContain(
93+
'channels.slack.channels."channel:customers" is ambiguous',
94+
);
95+
expect(nameKeyWarnings[3]).toContain('channels.slack.channels."CHANNEL:C0AL2GDUA7N"');
96+
expect(nameKeyWarnings[4]).toContain('channels.slack.channels."channel:C0al2gdua7p"');
97+
const dmWarnings = warnings.filter((warning) =>
98+
warning.includes("is a Slack DM conversation ID"),
99+
);
100+
expect(dmWarnings).toHaveLength(3);
101+
expect(dmWarnings[0]).toContain('channels.slack.channels."D0AL2GDUA7Q"');
102+
expect(dmWarnings[1]).toContain('channels.slack.channels."channel:d0al2gdua7r"');
103+
expect(dmWarnings[2]).toContain('channels.slack.channels."channel:dabcdefgh"');
104+
expect(dmWarnings[0]).toContain("channels.slack.dmPolicy");
105+
});
106+
107+
it("uses account policy and name-matching overrides for name-keyed channels (#81665)", async () => {
108+
const overlongName = "a".repeat(81);
109+
const warnings = await collectSlackWarnings({
110+
groupPolicy: "open",
111+
channels: { "root-room": {} },
112+
accounts: {
113+
inheritedOpen: {
114+
channels: { general: {} },
115+
},
116+
inheritedAllowlist: {
117+
groupPolicy: "allowlist",
118+
},
119+
explicitAllowlist: {
120+
groupPolicy: "allowlist",
121+
channels: { engineering: {} },
122+
},
123+
nameMatching: {
124+
groupPolicy: "allowlist",
125+
dangerouslyAllowNameMatching: true,
126+
channels: {
127+
support: {},
128+
"#help": {},
129+
"crème-brûlée": {},
130+
d0customers: {},
131+
dabcdefgh: {},
132+
"channel:customers": {},
133+
"<#C0AL2GDUA7J>": {},
134+
"slack:C0AL2GDUA7K": {},
135+
"@help": {},
136+
"##help": {},
137+
"help+": {},
138+
Support: {},
139+
"-": {},
140+
___: {},
141+
"#--": {},
142+
[overlongName]: {},
143+
},
144+
},
145+
},
146+
});
147+
148+
const nameKeyWarnings = warnings.filter((warning) =>
149+
warning.includes("Re-key it with the channel's"),
150+
);
151+
expect(nameKeyWarnings).toHaveLength(13);
152+
const rootWarning = nameKeyWarnings.find((warning) =>
153+
warning.includes('channels.slack.channels."root-room"'),
154+
);
155+
expect(rootWarning).toContain("messages from the channel are dropped");
156+
expect(
157+
nameKeyWarnings.some((warning) =>
158+
warning.includes('channels.slack.accounts.explicitAllowlist.channels."engineering"'),
159+
),
160+
).toBe(true);
161+
expect(
162+
nameKeyWarnings.some((warning) =>
163+
warning.includes(
164+
'channels.slack.accounts.nameMatching.channels."channel:customers" is ambiguous',
165+
),
166+
),
167+
).toBe(true);
168+
expect(
169+
nameKeyWarnings.some((warning) =>
170+
warning.includes('channels.slack.accounts.nameMatching.channels."<#C0AL2GDUA7J>"'),
171+
),
172+
).toBe(true);
173+
expect(
174+
nameKeyWarnings.some((warning) =>
175+
warning.includes('channels.slack.accounts.nameMatching.channels."slack:C0AL2GDUA7K"'),
176+
),
177+
).toBe(true);
178+
for (const invalidName of [
179+
"@help",
180+
"##help",
181+
"help+",
182+
"Support",
183+
"-",
184+
"___",
185+
"#--",
186+
overlongName,
187+
]) {
188+
expect(
189+
nameKeyWarnings.some((warning) =>
190+
warning.includes(`channels.slack.accounts.nameMatching.channels."${invalidName}"`),
191+
),
192+
).toBe(true);
193+
}
194+
195+
const sharedOpenWarnings = await collectSlackWarnings(
196+
{ channels: { "shared-room": {} } },
197+
{ groupPolicy: "open" },
198+
);
199+
expect(
200+
sharedOpenWarnings.some((warning) => warning.includes("not a routable Slack channel ID")),
201+
).toBe(true);
202+
});
203+
204+
it("warns when an open-policy override is keyed by channel name (#81665)", async () => {
205+
const warnings = await collectSlackWarnings({
206+
groupPolicy: "open",
207+
channels: {
208+
"private-room": { enabled: false },
209+
},
210+
});
211+
212+
expect(warnings).toEqual([expect.stringContaining('channels.slack.channels."private-room"')]);
213+
expect(warnings[0]).toContain("the channel remains allowed");
214+
});
215+
216+
it("warns for DM IDs regardless of room policy and uses account-scoped remediation", async () => {
217+
const openWarnings = await collectSlackWarnings({
218+
groupPolicy: "open",
219+
channels: {
220+
D0AL2GDUA7S: {},
221+
},
222+
});
223+
expect(openWarnings).toEqual([
224+
expect.stringContaining('channels.slack.channels."D0AL2GDUA7S"'),
225+
]);
226+
227+
const disabledAccountWarnings = await collectSlackWarnings({
228+
accounts: {
229+
work: {
230+
groupPolicy: "disabled",
231+
channels: {
232+
"channel:d0al2gdua7t": {},
233+
},
234+
},
235+
},
236+
});
237+
expect(disabledAccountWarnings).toEqual([
238+
expect.stringContaining('channels.slack.accounts.work.channels."channel:d0al2gdua7t"'),
239+
]);
240+
expect(disabledAccountWarnings[0]).toContain("channels.slack.accounts.work.dmPolicy");
241+
expect(disabledAccountWarnings[0]).toContain("channels.slack.accounts.work.allowFrom");
242+
243+
const inheritedChannelWarnings = await collectSlackWarnings({
244+
channels: {
245+
D0AL2GDUA7U: {},
246+
},
247+
accounts: {
248+
work: {
249+
groupPolicy: "disabled",
250+
dmPolicy: "allowlist",
251+
allowFrom: ["U0AL2GDUA7U"],
252+
},
253+
},
254+
});
255+
expect(inheritedChannelWarnings).toEqual([
256+
expect.stringContaining('channels.slack.channels."D0AL2GDUA7U"'),
257+
]);
258+
expect(inheritedChannelWarnings[0]).toContain("channels.slack.accounts.work.dmPolicy");
259+
});
260+
261+
it("treats bare lowercase D forms as ambiguous without name matching", async () => {
262+
const warnings = await collectSlackWarnings({
263+
channels: {
264+
d0customers: {},
265+
dabcdefgh: {},
266+
},
267+
});
268+
269+
expect(warnings).toHaveLength(2);
270+
expect(warnings[0]).toContain(
271+
'channels.slack.channels."d0customers" is ambiguous: it may be a lowercase Slack DM conversation ID or a channel name',
272+
);
273+
expect(warnings[1]).toContain(
274+
'channels.slack.channels."dabcdefgh" is ambiguous: it may be a lowercase Slack DM conversation ID or a channel name',
275+
);
276+
expect(warnings[0]).toContain("stable C/G ID");
277+
});
278+
279+
it("does not audit provider defaults as a standalone named account (#81665)", async () => {
280+
const warnings = await collectSlackWarnings({
281+
channels: {
282+
"provider-room": { enabled: false },
283+
},
284+
accounts: {
285+
work: {
286+
channels: {
287+
C0AL2GDUA7J: {},
288+
},
289+
},
290+
},
291+
});
292+
293+
expect(warnings.some((warning) => warning.includes("provider-room"))).toBe(false);
294+
});
295+
53296
it("normalizes legacy slack streaming aliases into the nested streaming shape", () => {
54297
const normalize = getSlackCompatibilityNormalizer();
55298

0 commit comments

Comments
 (0)