From a274ef929fb7a7b580f13ce53ea37add3dce4355 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 6 Mar 2026 11:08:45 -0500 Subject: [PATCH] Mattermost: harden interaction callback binding (#38057) --- extensions/mattermost/src/channel.ts | 15 ++- .../src/mattermost/interactions.test.ts | 96 ++++++++++++++++++- .../mattermost/src/mattermost/interactions.ts | 92 ++++++++++++------ .../mattermost/src/mattermost/monitor.ts | 2 +- extensions/mattermost/src/mattermost/send.ts | 46 +++++++-- 5 files changed, 212 insertions(+), 39 deletions(-) diff --git a/extensions/mattermost/src/channel.ts b/extensions/mattermost/src/channel.ts index 00e4c69e0f7..16df4f2ebcd 100644 --- a/extensions/mattermost/src/channel.ts +++ b/extensions/mattermost/src/channel.ts @@ -34,11 +34,13 @@ import { import { monitorMattermostProvider } from "./mattermost/monitor.js"; import { probeMattermost } from "./mattermost/probe.js"; import { addMattermostReaction, removeMattermostReaction } from "./mattermost/reactions.js"; -import { sendMessageMattermost } from "./mattermost/send.js"; +import { resolveMattermostSendChannelId, sendMessageMattermost } from "./mattermost/send.js"; import { looksLikeMattermostTargetId, normalizeMattermostMessagingTarget } from "./normalize.js"; import { mattermostOnboardingAdapter } from "./onboarding.js"; import { getMattermostRuntime } from "./runtime.js"; +const SIGNED_CHANNEL_ID_CONTEXT_KEY = "__openclaw_channel_id"; + const mattermostMessageActions: ChannelMessageActionAdapter = { listActions: ({ cfg }) => { const enabledAccounts = listMattermostAccountIds(cfg) @@ -165,6 +167,10 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { if (params.buttons && Array.isArray(params.buttons)) { const account = resolveMattermostAccount({ cfg, accountId: resolvedAccountId }); if (account.botToken) setInteractionSecret(account.accountId, account.botToken); + const channelId = await resolveMattermostSendChannelId(to, { + cfg, + accountId: account.accountId, + }); const callbackUrl = resolveInteractionCallbackUrl(account.accountId, { gateway: cfg.gateway, interactions: account.config.interactions, @@ -184,8 +190,11 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { style: (btn.style as "default" | "primary" | "danger") ?? "default", context: typeof btn.context === "object" && btn.context !== null - ? (btn.context as Record) - : undefined, + ? { + ...(btn.context as Record), + [SIGNED_CHANNEL_ID_CONTEXT_KEY]: channelId, + } + : { [SIGNED_CHANNEL_ID_CONTEXT_KEY]: channelId }, })) .filter((btn) => btn.id && btn.name); diff --git a/extensions/mattermost/src/mattermost/interactions.test.ts b/extensions/mattermost/src/mattermost/interactions.test.ts index 19d39676a27..9da60273d63 100644 --- a/extensions/mattermost/src/mattermost/interactions.test.ts +++ b/extensions/mattermost/src/mattermost/interactions.test.ts @@ -448,7 +448,7 @@ describe("createMattermostInteractionHandler", () => { } it("accepts non-localhost requests when the interaction token is valid", async () => { - const context = { action_id: "approve" }; + const context = { action_id: "approve", __openclaw_channel_id: "chan-1" }; const token = generateInteractionToken(context, "acct"); const requestLog: Array<{ path: string; method?: string }> = []; const handler = createMattermostInteractionHandler({ @@ -459,6 +459,7 @@ describe("createMattermostInteractionHandler", () => { return { id: "post-1" }; } return { + channel_id: "chan-1", message: "Choose", props: { attachments: [{ actions: [{ id: "approve", name: "Approve" }] }], @@ -516,4 +517,97 @@ describe("createMattermostInteractionHandler", () => { expect(res.statusCode).toBe(403); expect(res.body).toContain("Invalid token"); }); + + it("rejects requests when the signed channel does not match the callback payload", async () => { + const context = { action_id: "approve", __openclaw_channel_id: "chan-1" }; + const token = generateInteractionToken(context, "acct"); + const handler = createMattermostInteractionHandler({ + client: { + request: async () => ({ message: "unused" }), + } as unknown as MattermostClient, + botUserId: "bot", + accountId: "acct", + }); + + const req = createReq({ + body: { + user_id: "user-1", + channel_id: "chan-2", + post_id: "post-1", + context: { ...context, _token: token }, + }, + }); + const res = createRes(); + + await handler(req, res); + + expect(res.statusCode).toBe(403); + expect(res.body).toContain("Channel mismatch"); + }); + + it("rejects requests when the fetched post does not belong to the callback channel", async () => { + const context = { action_id: "approve", __openclaw_channel_id: "chan-1" }; + const token = generateInteractionToken(context, "acct"); + const handler = createMattermostInteractionHandler({ + client: { + request: async () => ({ + channel_id: "chan-9", + message: "Choose", + props: { + attachments: [{ actions: [{ id: "approve", name: "Approve" }] }], + }, + }), + } as unknown as MattermostClient, + botUserId: "bot", + accountId: "acct", + }); + + const req = createReq({ + body: { + user_id: "user-1", + channel_id: "chan-1", + post_id: "post-1", + context: { ...context, _token: token }, + }, + }); + const res = createRes(); + + await handler(req, res); + + expect(res.statusCode).toBe(403); + expect(res.body).toContain("Post/channel mismatch"); + }); + + it("rejects requests when the action is not present on the fetched post", async () => { + const context = { action_id: "approve", __openclaw_channel_id: "chan-1" }; + const token = generateInteractionToken(context, "acct"); + const handler = createMattermostInteractionHandler({ + client: { + request: async () => ({ + channel_id: "chan-1", + message: "Choose", + props: { + attachments: [{ actions: [{ id: "reject", name: "Reject" }] }], + }, + }), + } as unknown as MattermostClient, + botUserId: "bot", + accountId: "acct", + }); + + const req = createReq({ + body: { + user_id: "user-1", + channel_id: "chan-1", + post_id: "post-1", + context: { ...context, _token: token }, + }, + }); + const res = createRes(); + + await handler(req, res); + + expect(res.statusCode).toBe(403); + expect(res.body).toContain("Unknown action"); + }); }); diff --git a/extensions/mattermost/src/mattermost/interactions.ts b/extensions/mattermost/src/mattermost/interactions.ts index 33415ae519c..5ca911fbeb6 100644 --- a/extensions/mattermost/src/mattermost/interactions.ts +++ b/extensions/mattermost/src/mattermost/interactions.ts @@ -6,6 +6,7 @@ import { updateMattermostPost, type MattermostClient } from "./client.js"; const INTERACTION_MAX_BODY_BYTES = 64 * 1024; const INTERACTION_BODY_TIMEOUT_MS = 10_000; +const SIGNED_CHANNEL_ID_CONTEXT_KEY = "__openclaw_channel_id"; /** * Mattermost interactive message callback payload. @@ -363,6 +364,69 @@ export function createMattermostInteractionHandler(params: { return; } + const signedChannelId = + typeof contextWithoutToken[SIGNED_CHANNEL_ID_CONTEXT_KEY] === "string" + ? contextWithoutToken[SIGNED_CHANNEL_ID_CONTEXT_KEY].trim() + : ""; + if (signedChannelId && signedChannelId !== payload.channel_id) { + log?.( + `mattermost interaction: signed channel mismatch payload=${payload.channel_id} signed=${signedChannelId}`, + ); + res.statusCode = 403; + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify({ error: "Channel mismatch" })); + return; + } + + const userName = payload.user_name ?? payload.user_id; + let originalMessage = ""; + let clickedButtonName = actionId; + try { + const originalPost = await client.request<{ + channel_id?: string | null; + message?: string; + props?: Record; + }>(`/posts/${payload.post_id}`); + const postChannelId = originalPost.channel_id?.trim(); + if (!postChannelId || postChannelId !== payload.channel_id) { + log?.( + `mattermost interaction: post channel mismatch payload=${payload.channel_id} post=${postChannelId ?? ""}`, + ); + res.statusCode = 403; + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify({ error: "Post/channel mismatch" })); + return; + } + originalMessage = originalPost.message ?? ""; + + // Ensure the callback can only target an action that exists on the original post. + const postAttachments = Array.isArray(originalPost?.props?.attachments) + ? (originalPost.props.attachments as Array<{ + actions?: Array<{ id?: string; name?: string }>; + }>) + : []; + for (const att of postAttachments) { + const match = att.actions?.find((a) => a.id === actionId); + if (match?.name) { + clickedButtonName = match.name; + break; + } + } + if (clickedButtonName === actionId) { + log?.(`mattermost interaction: action ${actionId} not found in post ${payload.post_id}`); + res.statusCode = 403; + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify({ error: "Unknown action" })); + return; + } + } catch (err) { + log?.(`mattermost interaction: failed to validate post ${payload.post_id}: ${String(err)}`); + res.statusCode = 500; + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify({ error: "Failed to validate interaction" })); + return; + } + log?.( `mattermost interaction: action=${actionId} user=${payload.user_name ?? payload.user_id} ` + `post=${payload.post_id} channel=${payload.channel_id}`, @@ -389,34 +453,6 @@ export function createMattermostInteractionHandler(params: { log?.(`mattermost interaction: system event dispatch failed: ${String(err)}`); } - // Fetch the original post to preserve its message and find the clicked button name. - const userName = payload.user_name ?? payload.user_id; - let originalMessage = ""; - let clickedButtonName = actionId; // fallback to action ID if we can't find the name - try { - const originalPost = await client.request<{ - message?: string; - props?: Record; - }>(`/posts/${payload.post_id}`); - originalMessage = originalPost?.message ?? ""; - - // Find the clicked button's display name from the original attachments - const postAttachments = Array.isArray(originalPost?.props?.attachments) - ? (originalPost.props.attachments as Array<{ - actions?: Array<{ id?: string; name?: string }>; - }>) - : []; - for (const att of postAttachments) { - const match = att.actions?.find((a) => a.id === actionId); - if (match?.name) { - clickedButtonName = match.name; - break; - } - } - } catch (err) { - log?.(`mattermost interaction: failed to fetch post ${payload.post_id}: ${String(err)}`); - } - // Update the post via API to replace buttons with a completion indicator. try { await updateMattermostPost(client, payload.post_id, { diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 4ce11a6a003..e5a2c91263b 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -544,7 +544,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} Surface: "mattermost" as const, MessageSid: `interaction:${opts.postId}:${opts.actionId}`, WasMentioned: true, - CommandAuthorized: true, + CommandAuthorized: false, OriginatingChannel: "mattermost" as const, OriginatingTo: to, }); diff --git a/extensions/mattermost/src/mattermost/send.ts b/extensions/mattermost/src/mattermost/send.ts index 9011abbd27e..b4db4550c86 100644 --- a/extensions/mattermost/src/mattermost/send.ts +++ b/extensions/mattermost/src/mattermost/send.ts @@ -205,13 +205,19 @@ async function resolveTargetChannelId(params: { return channel.id; } -export async function sendMessageMattermost( +type MattermostSendContext = { + cfg: OpenClawConfig; + accountId: string; + token: string; + baseUrl: string; + channelId: string; +}; + +async function resolveMattermostSendContext( to: string, - text: string, opts: MattermostSendOpts = {}, -): Promise { +): Promise { const core = getCore(); - const logger = core.logging.getChildLogger({ module: "mattermost" }); const cfg = opts.cfg ?? core.config.loadConfig(); const account = resolveMattermostAccount({ cfg, @@ -237,6 +243,34 @@ export async function sendMessageMattermost( token, }); + return { + cfg, + accountId: account.accountId, + token, + baseUrl, + channelId, + }; +} + +export async function resolveMattermostSendChannelId( + to: string, + opts: MattermostSendOpts = {}, +): Promise { + return (await resolveMattermostSendContext(to, opts)).channelId; +} + +export async function sendMessageMattermost( + to: string, + text: string, + opts: MattermostSendOpts = {}, +): Promise { + const core = getCore(); + const logger = core.logging.getChildLogger({ module: "mattermost" }); + const { cfg, accountId, token, baseUrl, channelId } = await resolveMattermostSendContext( + to, + opts, + ); + const client = createMattermostClient({ baseUrl, botToken: token }); let message = text?.trim() ?? ""; let fileIds: string[] | undefined; @@ -269,7 +303,7 @@ export async function sendMessageMattermost( const tableMode = core.channel.text.resolveMarkdownTableMode({ cfg, channel: "mattermost", - accountId: account.accountId, + accountId, }); message = core.channel.text.convertMarkdownTables(message, tableMode); } @@ -291,7 +325,7 @@ export async function sendMessageMattermost( core.channel.activity.record({ channel: "mattermost", - accountId: account.accountId, + accountId, direction: "outbound", });