Skip to content

Commit d9e970e

Browse files
eleqtriziteleqtrizit
committed
fix(mattermost): pin channel type fail-closed tests
1 parent 94657be commit d9e970e

4 files changed

Lines changed: 238 additions & 2 deletions

File tree

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

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

3+
/** Returns the first non-empty trimmed Mattermost channel type, or null when none resolves. */
34
export function resolveMattermostChannelType(
45
...channelTypes: Array<string | null | undefined>
56
): string | null {
@@ -13,6 +14,7 @@ export function resolveMattermostChannelType(
1314
return null;
1415
}
1516

17+
/** Throws when the Mattermost channel type is missing or blank so auth callers fail closed. */
1618
export function mapMattermostChannelTypeToChatType(channelType?: string | null): ChatType {
1719
const channelTypeValue = resolveMattermostChannelType(channelType);
1820
if (!channelTypeValue) {
@@ -28,6 +30,7 @@ export function mapMattermostChannelTypeToChatType(channelType?: string | null):
2830
return "channel";
2931
}
3032

33+
/** Security-sensitive callers should pass channelType; fallback is only for non-auth routing. */
3134
export function resolveMattermostTrustedChatKind(params: {
3235
channelType?: string | null;
3336
fallback?: ChatType;

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, expect, it } from "vitest";
2-
import { mapMattermostChannelTypeToChatType } from "./monitor.js";
2+
import { mapMattermostChannelTypeToChatType, resolveMattermostTrustedChatKind } from "./monitor.js";
33

44
describe("mapMattermostChannelTypeToChatType", () => {
55
it("maps direct and group dm channel types", () => {
@@ -21,8 +21,23 @@ describe("mapMattermostChannelTypeToChatType", () => {
2121
expect(() => mapMattermostChannelTypeToChatType(undefined)).toThrow(
2222
"Mattermost channel type is required",
2323
);
24+
expect(() => mapMattermostChannelTypeToChatType(null)).toThrow(
25+
"Mattermost channel type is required",
26+
);
27+
expect(() => mapMattermostChannelTypeToChatType("")).toThrow(
28+
"Mattermost channel type is required",
29+
);
2430
expect(() => mapMattermostChannelTypeToChatType(" ")).toThrow(
2531
"Mattermost channel type is required",
2632
);
2733
});
34+
35+
it("requires an explicit channel type for trusted chat kind", () => {
36+
expect(() => resolveMattermostTrustedChatKind({})).toThrow(
37+
"Mattermost channel type is required",
38+
);
39+
expect(() => resolveMattermostTrustedChatKind({ channelType: undefined })).toThrow(
40+
"Mattermost channel type is required",
41+
);
42+
});
2843
});

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

Lines changed: 217 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { IncomingMessage, ServerResponse } from "node:http";
12
import { beforeEach, describe, expect, it, vi } from "vitest";
23
import type { OpenClawConfig, RuntimeEnv } from "./runtime-api.js";
34

@@ -80,6 +81,9 @@ const mockState = vi.hoisted(() => ({
8081
dispatchReplyFromConfig: vi.fn(),
8182
enqueueSystemEvent: vi.fn(),
8283
fetchMattermostMe: vi.fn(),
84+
interactionHandler: undefined as
85+
| ((req: IncomingMessage, res: ServerResponse) => Promise<boolean | void> | boolean | void)
86+
| undefined,
8387
registerMattermostMonitorSlashCommands: vi.fn(),
8488
registerPluginHttpRoute: vi.fn(),
8589
resolveChannelInfo: vi.fn(),
@@ -322,10 +326,79 @@ const testRuntime = (): RuntimeEnv =>
322326
}) as RuntimeEnv["exit"],
323327
}) satisfies RuntimeEnv;
324328

329+
function createInteractionRequest(params: {
330+
body: unknown;
331+
remoteAddress?: string;
332+
headers?: Record<string, string>;
333+
}): IncomingMessage {
334+
const body = JSON.stringify(params.body);
335+
const listeners = new Map<string, Array<(...args: unknown[]) => void>>();
336+
const req = {
337+
method: "POST",
338+
headers: params.headers ?? {},
339+
socket: { remoteAddress: params.remoteAddress ?? "127.0.0.1" },
340+
destroy: vi.fn(),
341+
on(event: string, handler: (...args: unknown[]) => void) {
342+
const existing = listeners.get(event) ?? [];
343+
existing.push(handler);
344+
listeners.set(event, existing);
345+
return this;
346+
},
347+
} as IncomingMessage & { emitTest: (event: string, ...args: unknown[]) => void };
348+
349+
req.emitTest = (event: string, ...args: unknown[]) => {
350+
const handlers = listeners.get(event) ?? [];
351+
for (const handler of handlers) {
352+
handler(...args);
353+
}
354+
};
355+
356+
queueMicrotask(() => {
357+
req.emitTest("data", Buffer.from(body));
358+
req.emitTest("end");
359+
});
360+
361+
return req;
362+
}
363+
364+
function createInteractionResponse(): ServerResponse & {
365+
headers: Record<string, string>;
366+
body: string;
367+
} {
368+
const res = {
369+
statusCode: 200,
370+
headers: {},
371+
body: "",
372+
setHeader(name: string, value: string | number | readonly string[]) {
373+
res.headers[name] = Array.isArray(value) ? value.join(",") : String(value);
374+
return res;
375+
},
376+
end(
377+
chunk?: string | Buffer | Uint8Array,
378+
_encoding?: BufferEncoding | (() => void),
379+
cb?: () => void,
380+
) {
381+
res.body = chunk ? String(chunk) : "";
382+
cb?.();
383+
return res;
384+
},
385+
} as ServerResponse & { headers: Record<string, string>; body: string };
386+
return res;
387+
}
388+
389+
async function runRegisteredInteractionHandler(body: unknown) {
390+
const handler = mockState.interactionHandler;
391+
expect(handler).toBeDefined();
392+
const res = createInteractionResponse();
393+
await handler?.(createInteractionRequest({ body }), res);
394+
return res;
395+
}
396+
325397
describe("mattermost inbound user posts", () => {
326398
beforeEach(() => {
327399
vi.clearAllMocks();
328400
mockState.abortController = undefined;
401+
mockState.interactionHandler = undefined;
329402
mockState.runtimeCore = createRuntimeCore(testConfig);
330403
mockState.createMattermostClient.mockReturnValue({});
331404
mockState.createMattermostDraftStream.mockReturnValue({
@@ -338,7 +411,12 @@ describe("mattermost inbound user posts", () => {
338411
update_at: 1,
339412
});
340413
mockState.registerMattermostMonitorSlashCommands.mockResolvedValue(undefined);
341-
mockState.registerPluginHttpRoute.mockReturnValue(vi.fn());
414+
mockState.registerPluginHttpRoute.mockImplementation(
415+
(params: { handler: NonNullable<typeof mockState.interactionHandler> }) => {
416+
mockState.interactionHandler = params.handler;
417+
return vi.fn();
418+
},
419+
);
342420
mockState.resolveChannelInfo.mockResolvedValue({
343421
id: "chan-1",
344422
name: "town-square",
@@ -405,6 +483,144 @@ describe("mattermost inbound user posts", () => {
405483
});
406484
});
407485

486+
it("drops user posts when channel type is unavailable from payload and lookup", async () => {
487+
const socket = new FakeWebSocket();
488+
const abortController = new AbortController();
489+
mockState.abortController = abortController;
490+
const restrictedDmConfig: OpenClawConfig = {
491+
channels: {
492+
mattermost: {
493+
enabled: true,
494+
baseUrl: "https://mattermost.example.com",
495+
botToken: "bot-token",
496+
chatmode: "onmessage",
497+
dmPolicy: "pairing",
498+
groupPolicy: "open",
499+
},
500+
},
501+
};
502+
mockState.runtimeCore = createRuntimeCore(restrictedDmConfig);
503+
mockState.resolveChannelInfo.mockResolvedValue(null);
504+
const { monitorMattermostProvider } = await import("./monitor.js");
505+
506+
const monitor = monitorMattermostProvider({
507+
config: restrictedDmConfig,
508+
runtime: testRuntime(),
509+
abortSignal: abortController.signal,
510+
webSocketFactory: () => socket,
511+
});
512+
513+
await vi.waitFor(() => {
514+
expect(socket.openListenerCount).toBeGreaterThan(0);
515+
});
516+
socket.emitOpen();
517+
518+
await socket.emitMessage({
519+
event: "posted",
520+
data: {
521+
channel_id: "dm-lookup-failed",
522+
sender_name: "mallory",
523+
post: JSON.stringify({
524+
id: "post-missing-type",
525+
channel_id: "dm-lookup-failed",
526+
user_id: "user-1",
527+
message: "direct hello",
528+
create_at: 1_714_000_000_000,
529+
}),
530+
},
531+
broadcast: {
532+
channel_id: "dm-lookup-failed",
533+
user_id: "user-1",
534+
},
535+
});
536+
socket.emitClose(1000);
537+
await monitor;
538+
539+
expect(mockState.resolveChannelInfo).toHaveBeenCalledWith("dm-lookup-failed");
540+
expect(mockState.dispatchReplyFromConfig).not.toHaveBeenCalled();
541+
expect(mockState.enqueueSystemEvent).not.toHaveBeenCalled();
542+
});
543+
544+
it("skips button system events and synthetic dispatch when channel lookup later fails", async () => {
545+
const socket = new FakeWebSocket();
546+
const abortController = new AbortController();
547+
mockState.abortController = abortController;
548+
const requestLog: string[] = [];
549+
mockState.createMattermostClient.mockReturnValue({
550+
request: vi.fn(async (path: string) => {
551+
requestLog.push(path);
552+
return {
553+
id: "post-button",
554+
channel_id: "button-channel",
555+
message: "Choose",
556+
props: {
557+
attachments: [
558+
{
559+
actions: [{ id: "approve", name: "Approve" }],
560+
},
561+
],
562+
},
563+
};
564+
}),
565+
});
566+
mockState.resolveChannelInfo
567+
.mockResolvedValueOnce({
568+
id: "button-channel",
569+
name: "town-square",
570+
display_name: "Town Square",
571+
team_id: "team-1",
572+
type: "O",
573+
})
574+
.mockResolvedValueOnce(null)
575+
.mockResolvedValueOnce(null);
576+
const { monitorMattermostProvider } = await import("./monitor.js");
577+
578+
const monitor = monitorMattermostProvider({
579+
config: testConfig,
580+
runtime: testRuntime(),
581+
abortSignal: abortController.signal,
582+
webSocketFactory: () => socket,
583+
});
584+
585+
await vi.waitFor(() => {
586+
expect(socket.openListenerCount).toBeGreaterThan(0);
587+
});
588+
socket.emitOpen();
589+
await vi.waitFor(() => {
590+
expect(mockState.interactionHandler).toBeDefined();
591+
});
592+
593+
const { generateInteractionToken } = await import("./interactions.js");
594+
const context = {
595+
action_id: "approve",
596+
__openclaw_channel_id: "button-channel",
597+
};
598+
const res = await runRegisteredInteractionHandler({
599+
user_id: "user-1",
600+
user_name: "mallory",
601+
channel_id: "button-channel",
602+
post_id: "post-button",
603+
context: {
604+
...context,
605+
_token: generateInteractionToken(context, "default"),
606+
},
607+
});
608+
socket.emitClose(1000);
609+
await monitor;
610+
611+
expect(res.statusCode).toBe(200);
612+
expect(res.body).toBe("{}");
613+
expect(requestLog).toEqual(["/posts/post-button"]);
614+
expect(mockState.resolveChannelInfo).toHaveBeenCalledTimes(3);
615+
expect(mockState.enqueueSystemEvent).not.toHaveBeenCalled();
616+
expect(mockState.dispatchReplyFromConfig).not.toHaveBeenCalled();
617+
expect(mockState.updateMattermostPost).toHaveBeenCalledWith(
618+
expect.anything(),
619+
"post-button",
620+
expect.objectContaining({ message: "Choose" }),
621+
);
622+
});
623+
408624
it("pins direct-message main route updates to the configured owner", async () => {
409625
const socket = new FakeWebSocket();
410626
const abortController = new AbortController();

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,6 +1277,8 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
12771277
}
12781278

12791279
const channelInfo = await resolveChannelInfo(channelId);
1280+
// Prefer the event channel_type because resolveChannelInfo can return cached null after
1281+
// the lookup-failure window from GHSA-gp79-m99v-gjmh; fail closed if neither resolves.
12801282
const channelType = resolveMattermostChannelType(
12811283
payload.data?.channel_type,
12821284
channelInfo?.type,

0 commit comments

Comments
 (0)