Skip to content

Commit 3d5e3cf

Browse files
steipetesteipete
authored
fix(ci): guard optional channel contracts
1 parent 1df6915 commit 3d5e3cf

7 files changed

Lines changed: 244 additions & 13 deletions

File tree

‎extensions/slack/src/monitor/auth.test.ts‎

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ function makeSlackCtx(allowFrom: string[]): SlackMonitorContext {
3131
function makeAuthorizeCtx(params?: {
3232
allowFrom?: string[];
3333
channelsConfig?: Record<string, { users?: string[] }>;
34+
dmPolicy?: SlackMonitorContext["dmPolicy"];
3435
resolveUserName?: (userId: string) => Promise<{ name?: string }>;
3536
resolveChannelName?: (
3637
channelId: string,
@@ -39,7 +40,7 @@ function makeAuthorizeCtx(params?: {
3940
return {
4041
allowFrom: params?.allowFrom ?? [],
4142
accountId: "main",
42-
dmPolicy: "open",
43+
dmPolicy: params?.dmPolicy ?? "open",
4344
dmEnabled: true,
4445
allowNameMatching: false,
4546
channelsConfig: params?.channelsConfig ?? {},
@@ -119,6 +120,7 @@ describe("authorizeSlackSystemEventSender", () => {
119120
});
120121

121122
beforeEach(() => {
123+
readChannelIngressStoreAllowFromForDmPolicyMock.mockReset();
122124
clearSlackAllowFromCacheForTest();
123125
delete process.env.OPENCLAW_SLACK_CHANNEL_MEMBERS_CACHE_TTL_MS;
124126
});
@@ -286,6 +288,44 @@ describe("authorizeSlackSystemEventSender", () => {
286288
});
287289
});
288290

291+
it("does not use pairing-store allowFrom for MPIM system events", async () => {
292+
readChannelIngressStoreAllowFromForDmPolicyMock.mockResolvedValue(["U_ATTACKER"]);
293+
const result = await authorizeSlackSystemEventSender({
294+
ctx: makeAuthorizeCtx({
295+
allowFrom: [],
296+
dmPolicy: "pairing",
297+
resolveChannelName: async () => ({ name: "group-dm", type: "mpim" }),
298+
}),
299+
senderId: "U_ATTACKER",
300+
channelId: "G_MPIM",
301+
});
302+
303+
expect(result).toEqual({
304+
allowed: false,
305+
reason: "sender-not-allowlisted",
306+
channelType: "mpim",
307+
channelName: "group-dm",
308+
});
309+
expect(readChannelIngressStoreAllowFromForDmPolicyMock).not.toHaveBeenCalled();
310+
});
311+
312+
it("allows MPIM system-event senders in the configured allowFrom", async () => {
313+
const result = await authorizeSlackSystemEventSender({
314+
ctx: makeAuthorizeCtx({
315+
allowFrom: ["U_OWNER"],
316+
resolveChannelName: async () => ({ name: "group-dm", type: "mpim" }),
317+
}),
318+
senderId: "U_OWNER",
319+
channelId: "G_MPIM",
320+
});
321+
322+
expect(result).toEqual({
323+
allowed: true,
324+
channelType: "mpim",
325+
channelName: "group-dm",
326+
});
327+
});
328+
289329
it("keeps channel users as the non-interactive gate even when global allowFrom is configured", async () => {
290330
const result = await authorizeSlackSystemEventSender({
291331
ctx: makeAuthorizeCtx({
@@ -479,6 +519,36 @@ describe("resolveSlackCommandIngress", () => {
479519
expect(result.commandAccess.authorized).toBe(false);
480520
expect(result.commandAccess.shouldBlockControlCommand).toBe(false);
481521
});
522+
523+
it("blocks MPIM senders outside the configured allowFrom", async () => {
524+
const result = await resolveSlackCommandIngress({
525+
ctx: makeAuthorizeCtx(),
526+
senderId: "U_ATTACKER",
527+
channelType: "mpim",
528+
channelId: "G_MPIM",
529+
ownerAllowFromLower: ["u_owner"],
530+
allowTextCommands: false,
531+
hasControlCommand: false,
532+
});
533+
534+
expect(result.senderAccess.decision).toBe("block");
535+
expect(result.senderAccess.gate?.allowed).toBe(false);
536+
});
537+
538+
it("allows MPIM senders in the configured allowFrom", async () => {
539+
const result = await resolveSlackCommandIngress({
540+
ctx: makeAuthorizeCtx(),
541+
senderId: "U_OWNER",
542+
channelType: "mpim",
543+
channelId: "G_MPIM",
544+
ownerAllowFromLower: ["u_owner"],
545+
allowTextCommands: false,
546+
hasControlCommand: false,
547+
});
548+
549+
expect(result.senderAccess.decision).toBe("allow");
550+
expect(result.senderAccess.gate?.allowed).toBe(true);
551+
});
482552
});
483553

484554
describe("authorizeSlackSystemEventSender interactiveEvent", () => {

‎extensions/slack/src/monitor/auth.ts‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,16 @@ export async function resolveSlackCommandIngress(params: {
377377
>["modeWhenAccessGroupsOff"];
378378
}) {
379379
const isDirectMessage = params.channelType === "im";
380+
const isGroupDm = params.channelType === "mpim";
380381
const channelUsers = normalizeAllowListLower(params.channelUsers);
381-
const channelUsersConfigured = !isDirectMessage && channelUsers.length > 0;
382+
const channelUsersConfigured = !isDirectMessage && !isGroupDm && channelUsers.length > 0;
383+
// MPIM ingress is group-shaped, but its sender policy is DM-owned. Callers
384+
// pass configured allowFrom without pairing-store approvals for this path.
385+
const groupAllowFrom = isGroupDm
386+
? params.ownerAllowFromLower
387+
: channelUsersConfigured
388+
? channelUsers
389+
: [];
382390
const result = await createSlackIngressResolver(params.ctx).message({
383391
subject: createSlackIngressSubject({
384392
senderId: params.senderId,
@@ -394,15 +402,15 @@ export async function resolveSlackCommandIngress(params: {
394402
mayPair: false,
395403
},
396404
dmPolicy: isDirectMessage ? "open" : "disabled",
397-
groupPolicy: channelUsersConfigured ? "allowlist" : "open",
405+
groupPolicy: isGroupDm || channelUsersConfigured ? "allowlist" : "open",
398406
policy: {
399407
groupAllowFromFallbackToAllowFrom: false,
400408
mutableIdentifierMatching: params.ctx.allowNameMatching ? "enabled" : "disabled",
401409
...(params.activation ? { activation: params.activation } : {}),
402410
},
403411
mentionFacts: params.mentionFacts,
404412
allowFrom: isDirectMessage ? ["*"] : params.ownerAllowFromLower,
405-
groupAllowFrom: channelUsersConfigured ? channelUsers : [],
413+
groupAllowFrom,
406414
command: {
407415
allowTextCommands: params.allowTextCommands,
408416
hasControlCommand: params.hasControlCommand,
@@ -424,8 +432,9 @@ async function decideSlackSystemIngress(params: {
424432
interactiveEvent: boolean;
425433
}): Promise<ChannelIngressDecision> {
426434
const isDirectMessage = params.channelType === "im";
435+
const isGroupDm = params.channelType === "mpim";
427436
const channelUsers = normalizeAllowListLower(params.channelUsers);
428-
const channelUsersConfigured = !isDirectMessage && channelUsers.length > 0;
437+
const channelUsersConfigured = !isDirectMessage && !isGroupDm && channelUsers.length > 0;
429438
const ownerAllowFrom =
430439
params.interactiveEvent && channelUsersConfigured
431440
? params.ownerAllowFromLower.filter((entry) => entry !== "*")
@@ -435,6 +444,9 @@ async function decideSlackSystemIngress(params: {
435444
if (isDirectMessage) {
436445
return [];
437446
}
447+
if (isGroupDm) {
448+
return ownerAllowFrom;
449+
}
438450
if (params.interactiveEvent && hasAnyCommandAllowlist) {
439451
return channelUsersConfigured ? channelUsers : [];
440452
}
@@ -458,8 +470,9 @@ async function decideSlackSystemIngress(params: {
458470
mayPair: false,
459471
},
460472
dmPolicy: isDirectMessage ? "open" : "disabled",
461-
groupPolicy:
462-
params.interactiveEvent && hasAnyCommandAllowlist
473+
groupPolicy: isGroupDm
474+
? "allowlist"
475+
: params.interactiveEvent && hasAnyCommandAllowlist
463476
? "open"
464477
: channelUsersConfigured || (!params.channelId && params.ownerAllowFromLower.length > 0)
465478
? "allowlist"

‎extensions/slack/src/monitor/events/interactions.test.ts‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1922,6 +1922,79 @@ describe("registerSlackInteractionEvents", () => {
19221922
});
19231923
});
19241924

1925+
it("blocks MPIM block actions when sender is outside configured allowFrom", async () => {
1926+
enqueueSystemEventMock.mockClear();
1927+
const { ctx, app, getHandler } = createContext({
1928+
allowFrom: ["U_OWNER"],
1929+
resolveChannelName: async () => ({ name: "group-dm", type: "mpim" }),
1930+
});
1931+
registerSlackInteractionEvents({ ctx: ctx as never });
1932+
const handler = getHandler();
1933+
1934+
const ack = vi.fn().mockResolvedValue(undefined);
1935+
const respond = vi.fn().mockResolvedValue(undefined);
1936+
await handler({
1937+
ack,
1938+
respond,
1939+
body: {
1940+
user: { id: "U_ATTACKER" },
1941+
channel: { id: "G_MPIM" },
1942+
message: {
1943+
ts: "311.312",
1944+
blocks: [{ type: "actions", block_id: "verify_block", elements: [] }],
1945+
},
1946+
},
1947+
action: {
1948+
type: "button",
1949+
action_id: "openclaw:verify",
1950+
block_id: "verify_block",
1951+
},
1952+
});
1953+
1954+
expect(ack).toHaveBeenCalled();
1955+
expect(enqueueSystemEventMock).not.toHaveBeenCalled();
1956+
expect(app.client.chat.update).not.toHaveBeenCalled();
1957+
expect(respond).toHaveBeenCalledWith({
1958+
text: "You are not authorized to use this control.",
1959+
response_type: "ephemeral",
1960+
});
1961+
});
1962+
1963+
it("allows MPIM block actions when sender is in configured allowFrom", async () => {
1964+
enqueueSystemEventMock.mockClear();
1965+
const { ctx, app, getHandler } = createContext({
1966+
allowFrom: ["U_OWNER"],
1967+
resolveChannelName: async () => ({ name: "group-dm", type: "mpim" }),
1968+
});
1969+
registerSlackInteractionEvents({ ctx: ctx as never });
1970+
const handler = getHandler();
1971+
1972+
const ack = vi.fn().mockResolvedValue(undefined);
1973+
const respond = vi.fn().mockResolvedValue(undefined);
1974+
await handler({
1975+
ack,
1976+
respond,
1977+
body: {
1978+
user: { id: "U_OWNER" },
1979+
channel: { id: "G_MPIM" },
1980+
message: {
1981+
ts: "313.314",
1982+
blocks: [{ type: "actions", block_id: "verify_block", elements: [] }],
1983+
},
1984+
},
1985+
action: {
1986+
type: "button",
1987+
action_id: "openclaw:verify",
1988+
block_id: "verify_block",
1989+
},
1990+
});
1991+
1992+
expect(ack).toHaveBeenCalled();
1993+
expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1);
1994+
expect(app.client.chat.update).toHaveBeenCalledTimes(1);
1995+
expect(respond).not.toHaveBeenCalled();
1996+
});
1997+
19251998
it("ignores malformed action payloads after ack and logs warning", async () => {
19261999
enqueueSystemEventMock.mockClear();
19272000
const { ctx, app, getHandler, runtimeLog } = createContext();

‎extensions/slack/src/monitor/message-handler/prepare.test.ts‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,7 @@ describe("slack prepareSlackMessage inbound contract", () => {
677677
channel: string;
678678
channelType: SlackMessageEvent["channel_type"];
679679
user: string;
680+
historyUser?: string;
680681
userName: string;
681682
starterText: string;
682683
followUpText: string;
@@ -696,16 +697,17 @@ describe("slack prepareSlackMessage inbound contract", () => {
696697

697698
async function prepareThreadContextAllowlistCase(params: ThreadContextAllowlistCaseParams) {
698699
const { storePath } = storeFixture.makeTmpStorePath();
700+
const historyUser = params.historyUser ?? params.user;
699701
const replies = vi
700702
.fn()
701703
.mockResolvedValueOnce({
702-
messages: [{ text: params.starterText, user: params.user, ts: params.startTs }],
704+
messages: [{ text: params.starterText, user: historyUser, ts: params.startTs }],
703705
})
704706
.mockResolvedValueOnce({
705707
messages: [
706-
{ text: params.starterText, user: params.user, ts: params.startTs },
708+
{ text: params.starterText, user: historyUser, ts: params.startTs },
707709
{ text: "assistant reply", bot_id: "B1", ts: params.replyTs },
708-
{ text: params.followUpText, user: params.user, ts: params.followUpTs },
710+
{ text: params.followUpText, user: historyUser, ts: params.followUpTs },
709711
{ text: "current message", user: params.user, ts: params.currentTs },
710712
],
711713
response_metadata: { next_cursor: "" },
@@ -1912,6 +1914,39 @@ Second paragraph should still reach the agent after Slack's preview cutoff.`;
19121914
expect(prepared.ctxPayload.From).toBe("slack:group:G123");
19131915
});
19141916

1917+
it("blocks MPIM messages from senders outside the configured allowFrom", async () => {
1918+
const ctx = createReplyToAllSlackCtx();
1919+
ctx.allowFrom = ["U_OWNER"];
1920+
const prepared = await prepareMessageWith(
1921+
ctx,
1922+
createSlackAccount({ replyToMode: "all" }),
1923+
createSlackMessage({
1924+
channel: "G123",
1925+
channel_type: "mpim",
1926+
user: "U_ATTACKER",
1927+
}),
1928+
);
1929+
1930+
expect(prepared).toBeNull();
1931+
});
1932+
1933+
it("allows MPIM messages from senders in the configured allowFrom", async () => {
1934+
const ctx = createReplyToAllSlackCtx();
1935+
ctx.allowFrom = ["U_OWNER"];
1936+
const prepared = await prepareMessageWith(
1937+
ctx,
1938+
createSlackAccount({ replyToMode: "all" }),
1939+
createSlackMessage({
1940+
channel: "G123",
1941+
channel_type: "mpim",
1942+
user: "U_OWNER",
1943+
}),
1944+
);
1945+
1946+
assertPrepared(prepared);
1947+
expect(prepared.ctxPayload.ChatType).toBe("group");
1948+
});
1949+
19151950
it("keeps one mpDM classification when a later event omits channel_type (#102676)", async () => {
19161951
const { account, conversationsInfo, ctx } = createMissingChannelInfoBotCtx();
19171952
// The real message ingress boundary records this before preparation starts.
@@ -2370,13 +2405,15 @@ Second paragraph should still reach the agent after Slack's preview cutoff.`;
23702405
channel: "G400",
23712406
channelType: "mpim",
23722407
user: "U4",
2408+
historyUser: "U5",
23732409
userName: "Evan",
23742410
starterText: "starter from mpim",
23752411
followUpText: "mpim follow-up",
23762412
startTs: "400.000",
23772413
replyTs: "400.500",
23782414
followUpTs: "400.800",
23792415
currentTs: "401.000",
2416+
allowFrom: ["U4"],
23802417
});
23812418

23822419
expectThreadContextAllowsHumanHistory(prepared, replies, "starter from mpim", "mpim follow-up");

‎extensions/slack/src/monitor/message-handler/prepare.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1041,8 +1041,8 @@ export async function prepareSlackMessage(params: {
10411041
},
10421042
});
10431043
const senderGate = messageIngress.senderAccess.gate;
1044-
if (isRoom && senderGate?.allowed === false) {
1045-
logVerbose(`Blocked unauthorized slack sender ${senderId} (not in channel users)`);
1044+
if (isRoomish && senderGate?.allowed === false) {
1045+
logVerbose(`Blocked unauthorized slack sender ${senderId} (not in sender allowlist)`);
10461046
return null;
10471047
}
10481048
if (

‎extensions/slack/src/monitor/slash.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1357,6 +1357,44 @@ describe("slack slash commands access groups", () => {
13571357
expect(dispatchArg?.ctx?.From).toBe("slack:group:G_MPIM");
13581358
});
13591359

1360+
it("blocks MPIM slash commands from senders outside the configured allowFrom", async () => {
1361+
const harness = createPolicyHarness({
1362+
allowFrom: ["U_OWNER"],
1363+
channelId: "G_MPIM",
1364+
channelName: "group-dm",
1365+
resolveChannelName: async () => ({ name: "group-dm", type: "mpim" }),
1366+
useAccessGroups: false,
1367+
});
1368+
const { respond } = await registerAndRunPolicySlash({
1369+
harness,
1370+
command: { user_id: "U_ATTACKER" },
1371+
});
1372+
1373+
expect(dispatchMock).not.toHaveBeenCalled();
1374+
expect(respond).toHaveBeenCalledWith({
1375+
text: "You are not authorized to use this command here.",
1376+
response_type: "ephemeral",
1377+
});
1378+
});
1379+
1380+
it("allows MPIM slash commands from senders in the configured allowFrom", async () => {
1381+
const harness = createPolicyHarness({
1382+
allowFrom: ["U_OWNER"],
1383+
channelId: "G_MPIM",
1384+
channelName: "group-dm",
1385+
resolveChannelName: async () => ({ name: "group-dm", type: "mpim" }),
1386+
});
1387+
const { respond } = await registerAndRunPolicySlash({
1388+
harness,
1389+
command: { user_id: "U_OWNER" },
1390+
});
1391+
1392+
expect(dispatchMock).toHaveBeenCalledTimes(1);
1393+
expect(responseTexts(respond)).not.toContain(
1394+
"You are not authorized to use this command here.",
1395+
);
1396+
});
1397+
13601398
it("enforces access-group gating when lookup fails for private channels", async () => {
13611399
const harness = createPolicyHarness({
13621400
allowFrom: [],

0 commit comments

Comments
 (0)