Skip to content

Commit 0c67dc7

Browse files
pgondhi987pgondhi987
authored
fix(mattermost): fail closed on missing channel type [AI] (#84091)
* fix: fail closed on missing Mattermost channel type * addressing codex review * docs: add changelog entry for PR merge
1 parent e98760a commit 0c67dc7

8 files changed

Lines changed: 192 additions & 13 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ Docs: https://docs.openclaw.ai
1111

1212
### Fixes
1313

14+
- fix(mattermost): fail closed on missing channel type [AI]. (#84091) Thanks @pgondhi987.
1415
- Recheck rebuilt system.run argv [AI]. (#84090) Thanks @pgondhi987.
1516
- Agents/messages: stop message-tool-only turns after a successful source-channel `message` send while keeping transcript mirrors under the session write lock. (#84289)
1617
- Agents: filter silent heartbeat response-tool transcript artifacts out of embedded context snapshots so later user turns are not polluted by heartbeat no-op messages. (#83477) Thanks @fuller-stack-dev.

‎extensions/mattermost/src/mattermost/monitor-auth.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -247,12 +247,12 @@ export async function authorizeMattermostCommandInvocation(params: {
247247
hasControlCommand,
248248
} = params;
249249

250-
if (!channelInfo) {
250+
if (!channelInfo?.type) {
251251
return {
252252
ok: false,
253253
denyReason: "unknown-channel",
254254
commandAuthorized: false,
255-
channelInfo: null,
255+
channelInfo,
256256
kind: "channel",
257257
chatType: "channel",
258258
channelName: "",

‎extensions/mattermost/src/mattermost/monitor-gating.test.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ describe("mattermost monitor gating", () => {
1111
expect(mapMattermostChannelTypeToChatType("G")).toBe("group");
1212
expect(mapMattermostChannelTypeToChatType("P")).toBe("group");
1313
expect(mapMattermostChannelTypeToChatType("O")).toBe("channel");
14-
expect(mapMattermostChannelTypeToChatType(undefined)).toBe("channel");
14+
expect(mapMattermostChannelTypeToChatType(undefined)).toBe("direct");
15+
expect(mapMattermostChannelTypeToChatType(null)).toBe("direct");
16+
expect(mapMattermostChannelTypeToChatType("")).toBe("direct");
1517
});
1618

1719
it("derives chat kind from trusted channel lookup before fallback state", () => {
@@ -28,7 +30,7 @@ describe("mattermost monitor gating", () => {
2830
}),
2931
).toBe("direct");
3032
expect(resolveMattermostTrustedChatKind({ fallback: "group" })).toBe("group");
31-
expect(resolveMattermostTrustedChatKind({})).toBe("channel");
33+
expect(resolveMattermostTrustedChatKind({})).toBe("direct");
3234
});
3335

3436
it("drops non-mentioned traffic when onchar is enabled but not triggered", () => {

‎extensions/mattermost/src/mattermost/monitor-gating.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
import type { ChatType, OpenClawConfig } from "./runtime-api.js";
22

33
export function mapMattermostChannelTypeToChatType(channelType?: string | null): ChatType {
4-
if (!channelType) {
5-
return "channel";
4+
const normalized = channelType?.trim().toUpperCase();
5+
if (!normalized) {
6+
return "direct";
67
}
7-
const normalized = channelType.trim().toUpperCase();
88
if (normalized === "D") {
99
return "direct";
1010
}
@@ -22,7 +22,7 @@ export function resolveMattermostTrustedChatKind(params: {
2222
if (channelType) {
2323
return mapMattermostChannelTypeToChatType(channelType);
2424
}
25-
return params.fallback ?? "channel";
25+
return params.fallback ?? "direct";
2626
}
2727

2828
export type MattermostRequireMentionResolverInput = {

‎extensions/mattermost/src/mattermost/monitor.authz.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,47 @@ describe("mattermost monitor authz", () => {
182182
});
183183
});
184184

185+
it("denies command invocations when channel type is unavailable", async () => {
186+
const decision = await authorizeMattermostCommandInvocation({
187+
account: {
188+
...accountFixture,
189+
config: {
190+
dmPolicy: "allowlist",
191+
groupPolicy: "open",
192+
allowFrom: ["trusted-user"],
193+
},
194+
},
195+
cfg: {},
196+
senderId: "new-user",
197+
senderName: "New User",
198+
channelId: "dm-1",
199+
channelInfo: {
200+
id: "dm-1",
201+
name: "",
202+
display_name: "",
203+
},
204+
storeAllowFrom: [],
205+
allowTextCommands: true,
206+
hasControlCommand: true,
207+
});
208+
209+
expect(decision).toEqual({
210+
ok: false,
211+
denyReason: "unknown-channel",
212+
commandAuthorized: false,
213+
channelInfo: {
214+
id: "dm-1",
215+
name: "",
216+
display_name: "",
217+
},
218+
kind: "channel",
219+
chatType: "channel",
220+
channelName: "",
221+
channelDisplay: "",
222+
roomLabel: "#dm-1",
223+
});
224+
});
225+
185226
it("authorizes group senders through static access groups", async () => {
186227
const decision = await authorizeMattermostCommandInvocation({
187228
account: {

‎extensions/mattermost/src/mattermost/monitor.channel-kind.test.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,14 @@ describe("mapMattermostChannelTypeToChatType", () => {
1212
expect(mapMattermostChannelTypeToChatType(" p ")).toBe("group");
1313
});
1414

15-
it("keeps public channels and unknown values as channel", () => {
15+
it("keeps public channels and unknown typed values as channel", () => {
1616
expect(mapMattermostChannelTypeToChatType("O")).toBe("channel");
1717
expect(mapMattermostChannelTypeToChatType("x")).toBe("channel");
18-
expect(mapMattermostChannelTypeToChatType(undefined)).toBe("channel");
18+
});
19+
20+
it("treats missing channel type as direct", () => {
21+
expect(mapMattermostChannelTypeToChatType(undefined)).toBe("direct");
22+
expect(mapMattermostChannelTypeToChatType(null)).toBe("direct");
23+
expect(mapMattermostChannelTypeToChatType("")).toBe("direct");
1924
});
2025
});

‎extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,117 @@ describe("mattermost inbound user posts", () => {
432432
expect(ctx?.Provider).toBe("mattermost");
433433
});
434434

435+
it("uses websocket channel type when REST channel lookup fails", async () => {
436+
const socket = new FakeWebSocket();
437+
const abortController = new AbortController();
438+
mockState.abortController = abortController;
439+
const runtimeCore = createRuntimeCore(testConfig);
440+
mockState.runtimeCore = runtimeCore;
441+
mockState.resolveChannelInfo.mockResolvedValue(null);
442+
443+
const monitor = monitorMattermostProvider({
444+
config: testConfig,
445+
runtime: testRuntime(),
446+
abortSignal: abortController.signal,
447+
webSocketFactory: () => socket,
448+
});
449+
450+
await vi.waitFor(() => {
451+
expect(socket.openListenerCount).toBeGreaterThan(0);
452+
});
453+
socket.emitOpen();
454+
455+
await socket.emitMessage({
456+
event: "posted",
457+
data: {
458+
channel_id: "chan-1",
459+
channel_name: "town-square",
460+
channel_display_name: "Town Square",
461+
channel_type: "O",
462+
sender_name: "alice",
463+
post: JSON.stringify({
464+
id: "post-ws-kind",
465+
channel_id: "chan-1",
466+
user_id: "user-1",
467+
message: "hello with websocket kind",
468+
create_at: 1_714_000_000_000,
469+
}),
470+
},
471+
broadcast: {
472+
channel_id: "chan-1",
473+
user_id: "user-1",
474+
},
475+
});
476+
socket.emitClose(1000);
477+
await monitor;
478+
479+
expect(mockState.dispatchReplyFromConfig).toHaveBeenCalledTimes(1);
480+
const ctx = mockState.dispatchReplyFromConfig.mock.calls.at(0)?.[0].ctx;
481+
expect(ctx?.BodyForAgent).toBe("hello with websocket kind");
482+
expect(ctx?.ChatType).toBe("channel");
483+
expect(ctx?.ConversationLabel).toBe("Town Square id:chan-1");
484+
expect(runtimeCore.channel.session.recordInboundSession).toHaveBeenCalledTimes(1);
485+
});
486+
487+
it("drops posts when neither REST nor websocket channel type can be resolved", async () => {
488+
const socket = new FakeWebSocket();
489+
const abortController = new AbortController();
490+
mockState.abortController = abortController;
491+
const channelTypeConfig: OpenClawConfig = {
492+
channels: {
493+
mattermost: {
494+
enabled: true,
495+
baseUrl: "https://mattermost.example.com",
496+
botToken: "bot-token",
497+
chatmode: "onmessage",
498+
dmPolicy: "allowlist",
499+
groupPolicy: "open",
500+
allowFrom: ["trusted-user"],
501+
},
502+
},
503+
};
504+
const runtimeCore = createRuntimeCore(channelTypeConfig);
505+
mockState.runtimeCore = runtimeCore;
506+
mockState.resolveChannelInfo.mockResolvedValue(null);
507+
508+
const monitor = monitorMattermostProvider({
509+
config: channelTypeConfig,
510+
runtime: testRuntime(),
511+
abortSignal: abortController.signal,
512+
webSocketFactory: () => socket,
513+
});
514+
515+
await vi.waitFor(() => {
516+
expect(socket.openListenerCount).toBeGreaterThan(0);
517+
});
518+
socket.emitOpen();
519+
520+
await socket.emitMessage({
521+
event: "posted",
522+
data: {
523+
channel_id: "dm-1",
524+
sender_name: "mallory",
525+
post: JSON.stringify({
526+
id: "post-missing-kind",
527+
channel_id: "dm-1",
528+
user_id: "new-user",
529+
message: "hello",
530+
create_at: 1_714_000_000_000,
531+
}),
532+
},
533+
broadcast: {
534+
channel_id: "dm-1",
535+
user_id: "new-user",
536+
},
537+
});
538+
abortController.abort();
539+
socket.emitClose(1000);
540+
await monitor;
541+
542+
expect(mockState.dispatchReplyFromConfig).not.toHaveBeenCalled();
543+
expect(runtimeCore.channel.session.recordInboundSession).not.toHaveBeenCalled();
544+
});
545+
435546
it("pins direct-message main route updates to the configured owner", async () => {
436547
const socket = new FakeWebSocket();
437548
const abortController = new AbortController();

‎extensions/mattermost/src/mattermost/monitor.ts‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,13 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
668668
},
669669
resolveSessionKey: async ({ channelId, userId, post }) => {
670670
const channelInfo = await resolveChannelInfo(channelId);
671-
const kind = mapMattermostChannelTypeToChatType(channelInfo?.type);
671+
if (!channelInfo?.type) {
672+
logVerboseMessage(
673+
`mattermost: drop interaction session event (cannot resolve channel type for ${channelId})`,
674+
);
675+
throw new Error("Mattermost channel type could not be resolved");
676+
}
677+
const kind = mapMattermostChannelTypeToChatType(channelInfo.type);
672678
const teamId = channelInfo?.team_id ?? undefined;
673679
const route = core.channel.routing.resolveAgentRoute({
674680
cfg,
@@ -691,7 +697,13 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
691697
},
692698
dispatchButtonClick: async (opts) => {
693699
const channelInfo = await resolveChannelInfo(opts.channelId);
694-
const kind = mapMattermostChannelTypeToChatType(channelInfo?.type);
700+
if (!channelInfo?.type) {
701+
logVerboseMessage(
702+
`mattermost: drop interaction dispatch (cannot resolve channel type for ${opts.channelId})`,
703+
);
704+
return;
705+
}
706+
const kind = mapMattermostChannelTypeToChatType(channelInfo.type);
695707
const chatType = channelChatType(kind);
696708
const teamId = channelInfo?.team_id ?? undefined;
697709
const channelName = channelInfo?.name ?? undefined;
@@ -1285,8 +1297,15 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
12851297
}
12861298

12871299
const channelInfo = await resolveChannelInfo(channelId);
1300+
const channelType =
1301+
normalizeOptionalString(channelInfo?.type) ??
1302+
normalizeOptionalString(payload.data?.channel_type);
1303+
if (!channelType) {
1304+
logVerboseMessage(`mattermost: drop post (cannot resolve channel type for ${channelId})`);
1305+
return;
1306+
}
12881307
const kind = resolveMattermostTrustedChatKind({
1289-
channelType: channelInfo?.type,
1308+
channelType,
12901309
});
12911310
const chatType = channelChatType(kind);
12921311

0 commit comments

Comments
 (0)