From 989126574ead75c0eedc185293659eb0d4fc6844 Mon Sep 17 00:00:00 2001 From: Muhammed Mukhthar CM Date: Tue, 3 Mar 2026 05:50:24 +0000 Subject: [PATCH] Mattermost: address slash command ownership and auth review findings --- .../mattermost/src/mattermost/monitor.ts | 70 ++++++----- .../src/mattermost/slash-commands.test.ts | 43 +++++++ .../src/mattermost/slash-commands.ts | 39 +++++- .../mattermost/src/mattermost/slash-http.ts | 111 ++++++++---------- 4 files changed, 164 insertions(+), 99 deletions(-) diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 172ebdcb335..6ad677cf131 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -299,49 +299,59 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} }); const allRegistered: import("./slash-commands.js").MattermostRegisteredCommand[] = []; + let teamRegistrationFailures = 0; - try { - for (const team of teams) { + for (const team of teams) { + try { const registered = await registerSlashCommands({ client, teamId: team.id, + creatorUserId: botUserId, callbackUrl, commands: dedupedCommands, log: (msg) => runtime.log?.(msg), }); allRegistered.push(...registered); + } catch (err) { + teamRegistrationFailures += 1; + runtime.error?.( + `mattermost: failed to register slash commands for team ${team.id}: ${String(err)}`, + ); } - } catch (err) { - // If we partially succeeded (some teams had commands created) but later failed, - // clean up the created commands so we don't strand registrations that will 503. - await cleanupSlashCommands({ - client, - commands: allRegistered, + } + + if (allRegistered.length === 0) { + runtime.error?.( + "mattermost: native slash commands enabled but no commands could be registered; keeping slash callbacks inactive", + ); + } else { + if (teamRegistrationFailures > 0) { + runtime.error?.( + `mattermost: slash command registration completed with ${teamRegistrationFailures} team error(s)`, + ); + } + + // Build trigger→originalName map for accurate command name resolution + const triggerMap = new Map(); + for (const cmd of dedupedCommands) { + if (cmd.originalName) { + triggerMap.set(cmd.trigger, cmd.originalName); + } + } + + activateSlashCommands({ + account, + commandTokens: allRegistered.map((cmd) => cmd.token).filter(Boolean), + registeredCommands: allRegistered, + triggerMap, + api: { cfg, runtime }, log: (msg) => runtime.log?.(msg), }); - throw err; + + runtime.log?.( + `mattermost: slash commands registered (${allRegistered.length} commands across ${teams.length} teams, callback=${callbackUrl})`, + ); } - - // Build trigger→originalName map for accurate command name resolution - const triggerMap = new Map(); - for (const cmd of dedupedCommands) { - if (cmd.originalName) { - triggerMap.set(cmd.trigger, cmd.originalName); - } - } - - activateSlashCommands({ - account, - commandTokens: allRegistered.map((cmd) => cmd.token).filter(Boolean), - registeredCommands: allRegistered, - triggerMap, - api: { cfg, runtime }, - log: (msg) => runtime.log?.(msg), - }); - - runtime.log?.( - `mattermost: slash commands registered (${allRegistered.length} commands across ${teams.length} teams, callback=${callbackUrl})`, - ); } catch (err) { runtime.error?.(`mattermost: failed to register slash commands: ${String(err)}`); } diff --git a/extensions/mattermost/src/mattermost/slash-commands.test.ts b/extensions/mattermost/src/mattermost/slash-commands.test.ts index bd127076fcf..39e4c1670d6 100644 --- a/extensions/mattermost/src/mattermost/slash-commands.test.ts +++ b/extensions/mattermost/src/mattermost/slash-commands.test.ts @@ -81,6 +81,7 @@ describe("slash-commands", () => { id: "cmd-1", token: "tok-1", team_id: "team-1", + creator_id: "bot-user", trigger: "oc_status", method: "P", url: "http://gateway/callback", @@ -95,6 +96,7 @@ describe("slash-commands", () => { const result = await registerSlashCommands({ client, teamId: "team-1", + creatorUserId: "bot-user", callbackUrl: "http://gateway/callback", commands: [ { @@ -110,4 +112,45 @@ describe("slash-commands", () => { expect(result[0]?.id).toBe("cmd-1"); expect(request).toHaveBeenCalledTimes(1); }); + + it("skips foreign command trigger collisions instead of mutating non-owned commands", async () => { + const request = vi.fn(async (path: string, init?: { method?: string }) => { + if (path.startsWith("/commands?team_id=")) { + return [ + { + id: "cmd-foreign-1", + token: "tok-foreign-1", + team_id: "team-1", + creator_id: "another-bot-user", + trigger: "oc_status", + method: "P", + url: "http://foreign/callback", + auto_complete: true, + }, + ]; + } + if (init?.method === "POST" || init?.method === "PUT" || init?.method === "DELETE") { + throw new Error("should not mutate foreign commands"); + } + throw new Error(`unexpected request path: ${path}`); + }); + const client = { request } as unknown as MattermostClient; + + const result = await registerSlashCommands({ + client, + teamId: "team-1", + creatorUserId: "bot-user", + callbackUrl: "http://gateway/callback", + commands: [ + { + trigger: "oc_status", + description: "status", + autoComplete: true, + }, + ], + }); + + expect(result).toHaveLength(0); + expect(request).toHaveBeenCalledTimes(1); + }); }); diff --git a/extensions/mattermost/src/mattermost/slash-commands.ts b/extensions/mattermost/src/mattermost/slash-commands.ts index 7181b02b788..89878289a6c 100644 --- a/extensions/mattermost/src/mattermost/slash-commands.ts +++ b/extensions/mattermost/src/mattermost/slash-commands.ts @@ -239,11 +239,16 @@ export async function updateMattermostCommand( export async function registerSlashCommands(params: { client: MattermostClient; teamId: string; + creatorUserId: string; callbackUrl: string; commands: MattermostCommandSpec[]; log?: (msg: string) => void; }): Promise { - const { client, teamId, callbackUrl, commands, log } = params; + const { client, teamId, creatorUserId, callbackUrl, commands, log } = params; + const normalizedCreatorUserId = creatorUserId.trim(); + if (!normalizedCreatorUserId) { + throw new Error("creatorUserId is required for slash command reconciliation"); + } // Fetch existing commands to avoid duplicates let existing: MattermostCommandResponse[] = []; @@ -257,12 +262,38 @@ export async function registerSlashCommands(params: { throw err; } - const existingByTrigger = new Map(existing.map((cmd) => [cmd.trigger, cmd])); + const existingByTrigger = new Map(); + for (const cmd of existing) { + const list = existingByTrigger.get(cmd.trigger) ?? []; + list.push(cmd); + existingByTrigger.set(cmd.trigger, list); + } const registered: MattermostRegisteredCommand[] = []; for (const spec of commands) { - const existingCmd = existingByTrigger.get(spec.trigger); + const existingForTrigger = existingByTrigger.get(spec.trigger) ?? []; + const ownedCommands = existingForTrigger.filter( + (cmd) => cmd.creator_id?.trim() === normalizedCreatorUserId, + ); + const foreignCommands = existingForTrigger.filter( + (cmd) => cmd.creator_id?.trim() !== normalizedCreatorUserId, + ); + + if (ownedCommands.length === 0 && foreignCommands.length > 0) { + log?.( + `mattermost: trigger /${spec.trigger} already used by non-OpenClaw command(s); skipping to avoid mutating external integrations`, + ); + continue; + } + + if (ownedCommands.length > 1) { + log?.( + `mattermost: multiple owned commands found for /${spec.trigger}; using the first and leaving extras untouched`, + ); + } + + const existingCmd = ownedCommands[0]; // Already registered with the correct callback URL if (existingCmd && existingCmd.url === callbackUrl) { @@ -307,7 +338,7 @@ export async function registerSlashCommands(params: { log?.( `mattermost: failed to update command /${spec.trigger} (id=${existingCmd.id}): ${String(err)}`, ); - // Fallback: try delete+recreate (safe for the oc_* namespace). + // Fallback: try delete+recreate for commands owned by this bot user. try { await deleteMattermostCommand(client, existingCmd.id); log?.(`mattermost: deleted stale command /${spec.trigger} (id=${existingCmd.id})`); diff --git a/extensions/mattermost/src/mattermost/slash-http.ts b/extensions/mattermost/src/mattermost/slash-http.ts index 963d1c9475d..a454b5c670a 100644 --- a/extensions/mattermost/src/mattermost/slash-http.ts +++ b/extensions/mattermost/src/mattermost/slash-http.ts @@ -10,6 +10,7 @@ import type { OpenClawConfig, ReplyPayload, RuntimeEnv } from "openclaw/plugin-s import { createReplyPrefixOptions, createTypingCallbacks, + isDangerousNameMatchingEnabled, logTypingFailure, resolveControlCommandGate, } from "openclaw/plugin-sdk"; @@ -23,6 +24,11 @@ import { sendMattermostTyping, type MattermostChannel, } from "./client.js"; +import { + isMattermostSenderAllowed, + normalizeMattermostAllowList, + resolveMattermostEffectiveAllowFromLists, +} from "./monitor-auth.js"; import { sendMessageMattermost } from "./send.js"; import { parseSlashCommandPayload, @@ -72,46 +78,6 @@ function sendJsonResponse( res.end(JSON.stringify(body)); } -/** - * Normalize a single allowlist entry, matching the websocket monitor behaviour. - * Strips `mattermost:`, `user:`, and `@` prefixes, and preserves the `*` wildcard. - */ -function normalizeAllowEntry(entry: string): string { - const trimmed = entry.trim(); - if (!trimmed) { - return ""; - } - if (trimmed === "*") { - return "*"; - } - return trimmed - .replace(/^(mattermost|user):/i, "") - .replace(/^@/, "") - .toLowerCase(); -} - -function normalizeAllowList(entries: Array): string[] { - const normalized = entries.map((entry) => normalizeAllowEntry(String(entry))).filter(Boolean); - return Array.from(new Set(normalized)); -} - -function isSenderAllowed(params: { senderId: string; senderName: string; allowFrom: string[] }) { - const { senderId, senderName, allowFrom } = params; - if (allowFrom.length === 0) { - return false; - } - if (allowFrom.includes("*")) { - return true; - } - - const normalizedId = normalizeAllowEntry(senderId); - const normalizedName = senderName ? normalizeAllowEntry(senderName) : ""; - - return allowFrom.some( - (entry) => entry === normalizedId || (normalizedName && entry === normalizedName), - ); -} - type SlashInvocationAuth = { ok: boolean; denyResponse?: MattermostSlashCommandResponse; @@ -181,10 +147,11 @@ async function authorizeSlashInvocation(params: { const dmPolicy = account.config.dmPolicy ?? "pairing"; const defaultGroupPolicy = cfg.channels?.defaults?.groupPolicy; const groupPolicy = account.config.groupPolicy ?? defaultGroupPolicy ?? "allowlist"; + const allowNameMatching = isDangerousNameMatchingEnabled(account.config); - const configAllowFrom = normalizeAllowList(account.config.allowFrom ?? []); - const configGroupAllowFrom = normalizeAllowList(account.config.groupAllowFrom ?? []); - const storeAllowFrom = normalizeAllowList( + const configAllowFrom = normalizeMattermostAllowList(account.config.allowFrom ?? []); + const configGroupAllowFrom = normalizeMattermostAllowList(account.config.groupAllowFrom ?? []); + const storeAllowFrom = normalizeMattermostAllowList( await core.channel.pairing .readAllowFromStore({ channel: "mattermost", @@ -192,13 +159,12 @@ async function authorizeSlashInvocation(params: { }) .catch(() => []), ); - const effectiveAllowFrom = Array.from(new Set([...configAllowFrom, ...storeAllowFrom])); - const effectiveGroupAllowFrom = Array.from( - new Set([ - ...(configGroupAllowFrom.length > 0 ? configGroupAllowFrom : configAllowFrom), - ...storeAllowFrom, - ]), - ); + const { effectiveAllowFrom, effectiveGroupAllowFrom } = resolveMattermostEffectiveAllowFromLists({ + allowFrom: configAllowFrom, + groupAllowFrom: configGroupAllowFrom, + storeAllowFrom, + dmPolicy, + }); const allowTextCommands = core.channel.commands.shouldHandleTextCommands({ cfg, @@ -206,24 +172,33 @@ async function authorizeSlashInvocation(params: { }); const hasControlCommand = core.channel.text.hasControlCommand(commandText, cfg); const useAccessGroups = cfg.commands?.useAccessGroups !== false; + const commandDmAllowFrom = kind === "direct" ? effectiveAllowFrom : configAllowFrom; + const commandGroupAllowFrom = + kind === "direct" + ? effectiveGroupAllowFrom + : configGroupAllowFrom.length > 0 + ? configGroupAllowFrom + : configAllowFrom; - const senderAllowedForCommands = isSenderAllowed({ + const senderAllowedForCommands = isMattermostSenderAllowed({ senderId, senderName, - allowFrom: effectiveAllowFrom, + allowFrom: commandDmAllowFrom, + allowNameMatching, }); - const groupAllowedForCommands = isSenderAllowed({ + const groupAllowedForCommands = isMattermostSenderAllowed({ senderId, senderName, - allowFrom: effectiveGroupAllowFrom, + allowFrom: commandGroupAllowFrom, + allowNameMatching, }); const commandGate = resolveControlCommandGate({ useAccessGroups, authorizers: [ - { configured: effectiveAllowFrom.length > 0, allowed: senderAllowedForCommands }, + { configured: commandDmAllowFrom.length > 0, allowed: senderAllowedForCommands }, { - configured: effectiveGroupAllowFrom.length > 0, + configured: commandGroupAllowFrom.length > 0, allowed: groupAllowedForCommands, }, ], @@ -661,16 +636,22 @@ async function handleSlashCommandAsync(params: { onReplyStart: typingCallbacks.onReplyStart, }); - await core.channel.reply.dispatchReplyFromConfig({ - ctx: ctxPayload, - cfg, + await core.channel.reply.withReplyDispatcher({ dispatcher, - replyOptions: { - ...replyOptions, - disableBlockStreaming: - typeof account.blockStreaming === "boolean" ? !account.blockStreaming : undefined, - onModelSelected, + onSettled: () => { + markDispatchIdle(); }, + run: () => + core.channel.reply.dispatchReplyFromConfig({ + ctx: ctxPayload, + cfg, + dispatcher, + replyOptions: { + ...replyOptions, + disableBlockStreaming: + typeof account.blockStreaming === "boolean" ? !account.blockStreaming : undefined, + onModelSelected, + }, + }), }); - markDispatchIdle(); }