Skip to content

Commit 6169e5d

Browse files
jontsaisteipete
jontsai
and
steipete
authored
fix(slack): allow channel-id reads for name-allowlisted channels (#95313)
* fix(slack): allow channel-id reads for name-allowlisted channels * fix(slack): trust API lookup for read target names * fix(slack): resolve name-allowlisted reads safely Co-authored-by: Jonathan Tsai <[email protected]> * fix(slack): resolve name-allowlisted reads safely * fix(slack): resolve name-allowlisted reads safely --------- Co-authored-by: Peter Steinberger <[email protected]>
1 parent e643828 commit 6169e5d

6 files changed

Lines changed: 249 additions & 20 deletions

File tree

‎extensions/slack/src/action-runtime.test.ts‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts";
33
import { beforeEach, describe, expect, it, vi } from "vitest";
44
import { handleSlackAction, slackActionRuntime } from "./action-runtime.js";
55
import { parseSlackBlocksInput } from "./blocks-input.js";
6+
import { buildSlackThreadingToolContext } from "./threading-tool-context.js";
67

78
const originalSlackActionRuntime = { ...slackActionRuntime };
89
const deleteSlackMessage = vi.fn(async (..._args: unknown[]) => ({}));
@@ -17,6 +18,9 @@ const reactSlackMessage = vi.fn(async (..._args: unknown[]) => ({}));
1718
const readSlackMessages = vi.fn(async (..._args: unknown[]) => ({}));
1819
const removeOwnSlackReactions = vi.fn(async (..._args: unknown[]) => ["thumbsup"]);
1920
const removeSlackReaction = vi.fn(async (..._args: unknown[]) => ({}));
21+
const resolveSlackConversationName = vi.fn(
22+
async (..._args: unknown[]): Promise<string | undefined> => undefined,
23+
);
2024
const sendSlackMessage = vi.fn(async (..._args: unknown[]) => ({ channelId: "C123" }));
2125
const unpinSlackMessage = vi.fn(async (..._args: unknown[]) => ({}));
2226

@@ -201,6 +205,7 @@ describe("handleSlackAction", () => {
201205

202206
beforeEach(() => {
203207
vi.clearAllMocks();
208+
resolveSlackConversationName.mockReset().mockResolvedValue(undefined);
204209
Object.assign(slackActionRuntime, originalSlackActionRuntime, {
205210
deleteSlackMessage,
206211
downloadSlackFile,
@@ -215,6 +220,7 @@ describe("handleSlackAction", () => {
215220
readSlackMessages,
216221
removeOwnSlackReactions,
217222
removeSlackReaction,
223+
resolveSlackConversationName,
218224
sendSlackMessage,
219225
unpinSlackMessage,
220226
});
@@ -994,6 +1000,152 @@ describe("handleSlackAction", () => {
9941000
expect(requireMockArg(readSlackMessages, "readSlackMessages", 0, 0)).toBe("C_ALLOWED");
9951001
});
9961002

1003+
it("resolves name-allowlisted reads from a core-shaped Slack threading context", async () => {
1004+
resolveSlackConversationName.mockResolvedValueOnce("allowed-channel");
1005+
readSlackMessages.mockResolvedValueOnce({ messages: [], hasMore: false });
1006+
1007+
const cfg = slackConfig({
1008+
groupPolicy: "allowlist",
1009+
dangerouslyAllowNameMatching: true,
1010+
channels: {
1011+
"#allowed-channel": { enabled: true },
1012+
},
1013+
});
1014+
const context = buildSlackThreadingToolContext({
1015+
cfg,
1016+
accountId: null,
1017+
context: {
1018+
ChatType: "channel",
1019+
Channel: "slack",
1020+
To: "channel:C0123456789",
1021+
},
1022+
});
1023+
1024+
await handleSlackAction({ action: "readMessages", channelId: "C0123456789" }, cfg, context);
1025+
1026+
expect(resolveSlackConversationName).toHaveBeenCalledWith("C0123456789", { cfg });
1027+
expect(requireMockArg(readSlackMessages, "readSlackMessages", 0, 0)).toBe("C0123456789");
1028+
});
1029+
1030+
it("does not treat the core Channel provider value as a Slack room name", async () => {
1031+
resolveSlackConversationName.mockResolvedValueOnce("actual-room");
1032+
1033+
const cfg = slackConfig({
1034+
groupPolicy: "allowlist",
1035+
dangerouslyAllowNameMatching: true,
1036+
channels: {
1037+
"#slack": { enabled: true },
1038+
},
1039+
});
1040+
const context = buildSlackThreadingToolContext({
1041+
cfg,
1042+
accountId: null,
1043+
context: {
1044+
ChatType: "channel",
1045+
Channel: "slack",
1046+
To: "channel:C0123456789",
1047+
},
1048+
});
1049+
1050+
await expect(
1051+
handleSlackAction({ action: "readMessages", channelId: "C0123456789" }, cfg, context),
1052+
).rejects.toThrow("Slack read target channel is not allowed.");
1053+
expect(resolveSlackConversationName).toHaveBeenCalledWith("C0123456789", { cfg });
1054+
expect(readSlackMessages).not.toHaveBeenCalled();
1055+
});
1056+
1057+
it("does not authorize different Slack targets with the current context channel ID", async () => {
1058+
resolveSlackConversationName.mockResolvedValueOnce("other-channel");
1059+
1060+
const cfg = slackConfig({
1061+
groupPolicy: "allowlist",
1062+
dangerouslyAllowNameMatching: true,
1063+
channels: {
1064+
"#allowed-channel": { enabled: true },
1065+
},
1066+
});
1067+
1068+
await expect(
1069+
handleSlackAction({ action: "readMessages", channelId: "C9876543210" }, cfg, {
1070+
currentChannelId: "C0123456789",
1071+
}),
1072+
).rejects.toThrow("Slack read target channel is not allowed.");
1073+
expect(resolveSlackConversationName).toHaveBeenCalledWith("C9876543210", { cfg });
1074+
expect(readSlackMessages).not.toHaveBeenCalled();
1075+
});
1076+
1077+
it("uses the configured user read token to resolve name-allowlisted channels", async () => {
1078+
resolveSlackConversationName.mockResolvedValueOnce("allowed-channel");
1079+
readSlackMessages.mockResolvedValueOnce({ messages: [], hasMore: false });
1080+
1081+
const cfg = slackConfig({
1082+
userToken: "xoxp-reader",
1083+
groupPolicy: "allowlist",
1084+
dangerouslyAllowNameMatching: true,
1085+
channels: {
1086+
"#allowed-channel": { enabled: true },
1087+
},
1088+
});
1089+
await handleSlackAction({ action: "readMessages", channelId: "C0123456789" }, cfg);
1090+
1091+
expect(resolveSlackConversationName).toHaveBeenCalledWith("C0123456789", {
1092+
cfg,
1093+
token: "xoxp-reader",
1094+
});
1095+
expect(requireMockArg(readSlackMessages, "readSlackMessages", 0, 0)).toBe("C0123456789");
1096+
});
1097+
1098+
it("resolves Slack target channel names before applying wildcard fallback denial", async () => {
1099+
resolveSlackConversationName.mockResolvedValueOnce("allowed-channel");
1100+
readSlackMessages.mockResolvedValueOnce({ messages: [], hasMore: false });
1101+
1102+
const cfg = slackConfig({
1103+
groupPolicy: "allowlist",
1104+
dangerouslyAllowNameMatching: true,
1105+
channels: {
1106+
"*": { enabled: false },
1107+
"#allowed-channel": { enabled: true },
1108+
},
1109+
});
1110+
await handleSlackAction({ action: "readMessages", channelId: "C0123456789" }, cfg);
1111+
1112+
expect(resolveSlackConversationName).toHaveBeenCalledWith("C0123456789", { cfg });
1113+
expect(requireMockArg(readSlackMessages, "readSlackMessages", 0, 0)).toBe("C0123456789");
1114+
});
1115+
1116+
it("does not let a name match override an explicit channel-id denial", async () => {
1117+
const cfg = slackConfig({
1118+
groupPolicy: "allowlist",
1119+
dangerouslyAllowNameMatching: true,
1120+
channels: {
1121+
C0123456789: { enabled: false },
1122+
"#allowed-channel": { enabled: true },
1123+
},
1124+
});
1125+
1126+
await expect(
1127+
handleSlackAction({ action: "readMessages", channelId: "C0123456789" }, cfg),
1128+
).rejects.toThrow("Slack read target channel is not allowed.");
1129+
expect(resolveSlackConversationName).not.toHaveBeenCalled();
1130+
expect(readSlackMessages).not.toHaveBeenCalled();
1131+
});
1132+
1133+
it("fails closed before reading when Slack cannot resolve the target name", async () => {
1134+
resolveSlackConversationName.mockRejectedValueOnce(new Error("missing_scope"));
1135+
const cfg = slackConfig({
1136+
groupPolicy: "allowlist",
1137+
dangerouslyAllowNameMatching: true,
1138+
channels: {
1139+
"#allowed-channel": { enabled: true },
1140+
},
1141+
});
1142+
1143+
await expect(
1144+
handleSlackAction({ action: "readMessages", channelId: "C0123456789" }, cfg),
1145+
).rejects.toThrow("missing_scope");
1146+
expect(readSlackMessages).not.toHaveBeenCalled();
1147+
});
1148+
9971149
it("rejects Slack reads for non-allowlisted target channels", async () => {
9981150
const cfg = slackConfig({
9991151
groupPolicy: "allowlist",

‎extensions/slack/src/action-runtime.ts‎

Lines changed: 57 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ export const slackActionRuntime = {
6262
readSlackMessages: createLazySlackAction("readSlackMessages"),
6363
removeOwnSlackReactions: createLazySlackAction("removeOwnSlackReactions"),
6464
removeSlackReaction: createLazySlackAction("removeSlackReaction"),
65+
resolveSlackConversationName: createLazySlackAction("resolveSlackConversationName"),
6566
sendSlackMessage: createLazySlackAction("sendSlackMessage"),
6667
unpinSlackMessage: createLazySlackAction("unpinSlackMessage"),
6768
};
@@ -143,15 +144,19 @@ function isImageContentType(value: string | undefined): boolean {
143144
return value?.trim().toLowerCase().startsWith("image/") === true;
144145
}
145146

146-
function assertSlackReadTargetAllowed(params: {
147+
type SlackReadTargetDecision = "allow" | "deny" | "resolve-name";
148+
149+
function resolveSlackReadTargetDecision(params: {
147150
account: ResolvedSlackAccount;
148151
cfg: OpenClawConfig;
149152
channelId: string;
150-
}) {
153+
channelName?: string;
154+
}): SlackReadTargetDecision {
151155
const channels = params.account.config.channels;
152156
const channelKeys = Object.keys(channels ?? {});
153157
const channelConfig = resolveSlackChannelConfig({
154158
channelId: params.channelId,
159+
channelName: params.channelName,
155160
channels,
156161
channelKeys,
157162
allowNameMatching: params.account.config.dangerouslyAllowNameMatching,
@@ -163,20 +168,44 @@ function assertSlackReadTargetAllowed(params: {
163168
groupPolicy: params.account.config.groupPolicy,
164169
defaultGroupPolicy: params.cfg.channels?.defaults?.groupPolicy,
165170
});
166-
if (
167-
groupPolicy === "disabled" ||
168-
(groupPolicy === "allowlist" &&
169-
!isSlackChannelAllowedByPolicy({
170-
groupPolicy,
171-
channelAllowlistConfigured: channelKeys.length > 0,
172-
channelAllowed,
173-
}))
174-
) {
175-
throw new Error("Slack read target channel is not allowed.");
171+
const policyAllowed = isSlackChannelAllowedByPolicy({
172+
groupPolicy,
173+
channelAllowlistConfigured: channelKeys.length > 0,
174+
channelAllowed,
175+
});
176+
if (policyAllowed) {
177+
return !channelAllowed && (groupPolicy !== "open" || channelConfig?.matchSource)
178+
? "deny"
179+
: "allow";
176180
}
177-
if (!channelAllowed && (groupPolicy !== "open" || channelConfig?.matchSource)) {
178-
throw new Error("Slack read target channel is not allowed.");
181+
182+
const canResolveName =
183+
groupPolicy === "allowlist" &&
184+
channelKeys.length > 0 &&
185+
params.account.config.dangerouslyAllowNameMatching === true &&
186+
!params.channelName &&
187+
(channelConfig?.matchSource === undefined || channelConfig.matchSource === "wildcard");
188+
return canResolveName ? "resolve-name" : "deny";
189+
}
190+
191+
async function assertSlackReadTargetAllowed(params: {
192+
account: ResolvedSlackAccount;
193+
cfg: OpenClawConfig;
194+
channelId: string;
195+
resolveChannelName: () => Promise<string | undefined>;
196+
}) {
197+
const direct = resolveSlackReadTargetDecision(params);
198+
if (direct === "allow") {
199+
return;
200+
}
201+
if (direct === "resolve-name") {
202+
const channelName = await params.resolveChannelName();
203+
if (channelName && resolveSlackReadTargetDecision({ ...params, channelName }) === "allow") {
204+
return;
205+
}
179206
}
207+
208+
throw new Error("Slack read target channel is not allowed.");
180209
}
181210

182211
export async function handleSlackAction(
@@ -223,6 +252,16 @@ export async function handleSlackAction(
223252

224253
const readOpts = buildActionOpts("read");
225254
const writeOpts = buildActionOpts("write");
255+
const assertReadTargetAllowed = async (channelId: string) =>
256+
await assertSlackReadTargetAllowed({
257+
account,
258+
cfg,
259+
channelId,
260+
// Use the same credential that will perform the authorized read. Slack
261+
// exposes conversation metadata according to the presented token's access.
262+
resolveChannelName: async () =>
263+
await slackActionRuntime.resolveSlackConversationName(channelId, readOpts),
264+
});
226265

227266
if (reactionsActions.has(action)) {
228267
if (!isActionEnabled("reactions")) {
@@ -255,7 +294,7 @@ export async function handleSlackAction(
255294
}
256295
return jsonResult({ ok: true, added: emoji });
257296
}
258-
assertSlackReadTargetAllowed({ account, cfg, channelId });
297+
await assertReadTargetAllowed(channelId);
259298
const reactions = readOpts
260299
? await slackActionRuntime.listSlackReactions(channelId, messageId, readOpts)
261300
: await slackActionRuntime.listSlackReactions(channelId, messageId);
@@ -404,7 +443,7 @@ export async function handleSlackAction(
404443
}
405444
case "readMessages": {
406445
const channelId = resolveChannelId();
407-
assertSlackReadTargetAllowed({ account, cfg, channelId });
446+
await assertReadTargetAllowed(channelId);
408447
const limit = readPositiveIntegerParam(params, "limit", {
409448
message: "limit must be a positive integer.",
410449
});
@@ -440,7 +479,7 @@ export async function handleSlackAction(
440479
);
441480
}
442481
const channelId = resolveSlackChannelId(channelTarget);
443-
assertSlackReadTargetAllowed({ account, cfg, channelId });
482+
await assertReadTargetAllowed(channelId);
444483
const threadId = readStringParam(params, "threadId") ?? readStringParam(params, "replyTo");
445484
const maxBytes = account.config?.mediaMaxMb
446485
? account.config.mediaMaxMb * 1024 * 1024
@@ -517,7 +556,7 @@ export async function handleSlackAction(
517556
}
518557
return jsonResult({ ok: true });
519558
}
520-
assertSlackReadTargetAllowed({ account, cfg, channelId });
559+
await assertReadTargetAllowed(channelId);
521560
const pins = writeOpts
522561
? await slackActionRuntime.listSlackPins(channelId, readOpts)
523562
: await slackActionRuntime.listSlackPins(channelId);

‎extensions/slack/src/actions.read.test.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,41 @@
11
// Slack tests cover actions.read plugin behavior.
22
import type { WebClient } from "@slack/web-api";
33
import { describe, expect, it, vi } from "vitest";
4-
import { readSlackMessages } from "./actions.js";
4+
import { readSlackMessages, resolveSlackConversationName } from "./actions.js";
55

66
function createClient() {
77
return {
88
conversations: {
9+
info: vi.fn(async () => ({ channel: { name: "general" } })),
910
replies: vi.fn(async () => ({ messages: [], has_more: false })),
1011
history: vi.fn(async () => ({ messages: [], has_more: false })),
1112
},
1213
} as unknown as WebClient & {
1314
conversations: {
15+
info: ReturnType<typeof vi.fn>;
1416
replies: ReturnType<typeof vi.fn>;
1517
history: ReturnType<typeof vi.fn>;
1618
};
1719
};
1820
}
1921

20-
describe("readSlackMessages", () => {
22+
describe("Slack read actions", () => {
23+
it("resolves the current Slack conversation name without caching failures", async () => {
24+
const client = createClient();
25+
client.conversations.info
26+
.mockRejectedValueOnce(new Error("temporary_failure"))
27+
.mockResolvedValueOnce({ channel: { name: " allowed-channel " } });
28+
29+
await expect(
30+
resolveSlackConversationName("C1", { client, token: "xoxp-reader" }),
31+
).rejects.toThrow("temporary_failure");
32+
await expect(
33+
resolveSlackConversationName("C1", { client, token: "xoxp-reader" }),
34+
).resolves.toBe("allowed-channel");
35+
expect(client.conversations.info).toHaveBeenNthCalledWith(1, { channel: "C1" });
36+
expect(client.conversations.info).toHaveBeenNthCalledWith(2, { channel: "C1" });
37+
});
38+
2139
it("uses conversations.replies and drops the parent message", async () => {
2240
const client = createClient();
2341
client.conversations.replies.mockResolvedValueOnce({

‎extensions/slack/src/actions.runtime.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ export {
1212
readSlackMessages,
1313
removeOwnSlackReactions,
1414
removeSlackReaction,
15+
resolveSlackConversationName,
1516
sendSlackMessage,
1617
unpinSlackMessage,
1718
} from "./actions.js";

‎extensions/slack/src/actions.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,15 @@ export async function deleteSlackMessage(
296296
});
297297
}
298298

299+
export async function resolveSlackConversationName(
300+
channelId: string,
301+
opts: SlackActionClientOpts = {},
302+
): Promise<string | undefined> {
303+
const client = await getClient(opts, "read");
304+
const info = await client.conversations.info({ channel: channelId });
305+
return info.channel?.name?.trim() || undefined;
306+
}
307+
299308
export async function readSlackMessages(
300309
channelId: string,
301310
opts: SlackActionClientOpts & {

0 commit comments

Comments
 (0)