Skip to content

Commit 9891265

Browse files
mukhtharcmmukhtharcm
committed
Mattermost: address slash command ownership and auth review findings
1 parent 67f20c6 commit 9891265

4 files changed

Lines changed: 162 additions & 97 deletions

File tree

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

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -299,49 +299,59 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {}
299299
});
300300

301301
const allRegistered: import("./slash-commands.js").MattermostRegisteredCommand[] = [];
302+
let teamRegistrationFailures = 0;
302303

303-
try {
304-
for (const team of teams) {
304+
for (const team of teams) {
305+
try {
305306
const registered = await registerSlashCommands({
306307
client,
307308
teamId: team.id,
309+
creatorUserId: botUserId,
308310
callbackUrl,
309311
commands: dedupedCommands,
310312
log: (msg) => runtime.log?.(msg),
311313
});
312314
allRegistered.push(...registered);
315+
} catch (err) {
316+
teamRegistrationFailures += 1;
317+
runtime.error?.(
318+
`mattermost: failed to register slash commands for team ${team.id}: ${String(err)}`,
319+
);
313320
}
314-
} catch (err) {
315-
// If we partially succeeded (some teams had commands created) but later failed,
316-
// clean up the created commands so we don't strand registrations that will 503.
317-
await cleanupSlashCommands({
318-
client,
319-
commands: allRegistered,
320-
log: (msg) => runtime.log?.(msg),
321-
});
322-
throw err;
323321
}
324322

325-
// Build trigger→originalName map for accurate command name resolution
326-
const triggerMap = new Map<string, string>();
327-
for (const cmd of dedupedCommands) {
328-
if (cmd.originalName) {
329-
triggerMap.set(cmd.trigger, cmd.originalName);
323+
if (allRegistered.length === 0) {
324+
runtime.error?.(
325+
"mattermost: native slash commands enabled but no commands could be registered; keeping slash callbacks inactive",
326+
);
327+
} else {
328+
if (teamRegistrationFailures > 0) {
329+
runtime.error?.(
330+
`mattermost: slash command registration completed with ${teamRegistrationFailures} team error(s)`,
331+
);
330332
}
331-
}
332333

333-
activateSlashCommands({
334-
account,
335-
commandTokens: allRegistered.map((cmd) => cmd.token).filter(Boolean),
336-
registeredCommands: allRegistered,
337-
triggerMap,
338-
api: { cfg, runtime },
339-
log: (msg) => runtime.log?.(msg),
340-
});
334+
// Build trigger→originalName map for accurate command name resolution
335+
const triggerMap = new Map<string, string>();
336+
for (const cmd of dedupedCommands) {
337+
if (cmd.originalName) {
338+
triggerMap.set(cmd.trigger, cmd.originalName);
339+
}
340+
}
341341

342-
runtime.log?.(
343-
`mattermost: slash commands registered (${allRegistered.length} commands across ${teams.length} teams, callback=${callbackUrl})`,
344-
);
342+
activateSlashCommands({
343+
account,
344+
commandTokens: allRegistered.map((cmd) => cmd.token).filter(Boolean),
345+
registeredCommands: allRegistered,
346+
triggerMap,
347+
api: { cfg, runtime },
348+
log: (msg) => runtime.log?.(msg),
349+
});
350+
351+
runtime.log?.(
352+
`mattermost: slash commands registered (${allRegistered.length} commands across ${teams.length} teams, callback=${callbackUrl})`,
353+
);
354+
}
345355
} catch (err) {
346356
runtime.error?.(`mattermost: failed to register slash commands: ${String(err)}`);
347357
}

‎extensions/mattermost/src/mattermost/slash-commands.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ describe("slash-commands", () => {
8181
id: "cmd-1",
8282
token: "tok-1",
8383
team_id: "team-1",
84+
creator_id: "bot-user",
8485
trigger: "oc_status",
8586
method: "P",
8687
url: "http://gateway/callback",
@@ -95,6 +96,7 @@ describe("slash-commands", () => {
9596
const result = await registerSlashCommands({
9697
client,
9798
teamId: "team-1",
99+
creatorUserId: "bot-user",
98100
callbackUrl: "http://gateway/callback",
99101
commands: [
100102
{
@@ -110,4 +112,45 @@ describe("slash-commands", () => {
110112
expect(result[0]?.id).toBe("cmd-1");
111113
expect(request).toHaveBeenCalledTimes(1);
112114
});
115+
116+
it("skips foreign command trigger collisions instead of mutating non-owned commands", async () => {
117+
const request = vi.fn(async (path: string, init?: { method?: string }) => {
118+
if (path.startsWith("/commands?team_id=")) {
119+
return [
120+
{
121+
id: "cmd-foreign-1",
122+
token: "tok-foreign-1",
123+
team_id: "team-1",
124+
creator_id: "another-bot-user",
125+
trigger: "oc_status",
126+
method: "P",
127+
url: "http://foreign/callback",
128+
auto_complete: true,
129+
},
130+
];
131+
}
132+
if (init?.method === "POST" || init?.method === "PUT" || init?.method === "DELETE") {
133+
throw new Error("should not mutate foreign commands");
134+
}
135+
throw new Error(`unexpected request path: ${path}`);
136+
});
137+
const client = { request } as unknown as MattermostClient;
138+
139+
const result = await registerSlashCommands({
140+
client,
141+
teamId: "team-1",
142+
creatorUserId: "bot-user",
143+
callbackUrl: "http://gateway/callback",
144+
commands: [
145+
{
146+
trigger: "oc_status",
147+
description: "status",
148+
autoComplete: true,
149+
},
150+
],
151+
});
152+
153+
expect(result).toHaveLength(0);
154+
expect(request).toHaveBeenCalledTimes(1);
155+
});
113156
});

‎extensions/mattermost/src/mattermost/slash-commands.ts‎

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,16 @@ export async function updateMattermostCommand(
239239
export async function registerSlashCommands(params: {
240240
client: MattermostClient;
241241
teamId: string;
242+
creatorUserId: string;
242243
callbackUrl: string;
243244
commands: MattermostCommandSpec[];
244245
log?: (msg: string) => void;
245246
}): Promise<MattermostRegisteredCommand[]> {
246-
const { client, teamId, callbackUrl, commands, log } = params;
247+
const { client, teamId, creatorUserId, callbackUrl, commands, log } = params;
248+
const normalizedCreatorUserId = creatorUserId.trim();
249+
if (!normalizedCreatorUserId) {
250+
throw new Error("creatorUserId is required for slash command reconciliation");
251+
}
247252

248253
// Fetch existing commands to avoid duplicates
249254
let existing: MattermostCommandResponse[] = [];
@@ -257,12 +262,38 @@ export async function registerSlashCommands(params: {
257262
throw err;
258263
}
259264

260-
const existingByTrigger = new Map(existing.map((cmd) => [cmd.trigger, cmd]));
265+
const existingByTrigger = new Map<string, MattermostCommandResponse[]>();
266+
for (const cmd of existing) {
267+
const list = existingByTrigger.get(cmd.trigger) ?? [];
268+
list.push(cmd);
269+
existingByTrigger.set(cmd.trigger, list);
270+
}
261271

262272
const registered: MattermostRegisteredCommand[] = [];
263273

264274
for (const spec of commands) {
265-
const existingCmd = existingByTrigger.get(spec.trigger);
275+
const existingForTrigger = existingByTrigger.get(spec.trigger) ?? [];
276+
const ownedCommands = existingForTrigger.filter(
277+
(cmd) => cmd.creator_id?.trim() === normalizedCreatorUserId,
278+
);
279+
const foreignCommands = existingForTrigger.filter(
280+
(cmd) => cmd.creator_id?.trim() !== normalizedCreatorUserId,
281+
);
282+
283+
if (ownedCommands.length === 0 && foreignCommands.length > 0) {
284+
log?.(
285+
`mattermost: trigger /${spec.trigger} already used by non-OpenClaw command(s); skipping to avoid mutating external integrations`,
286+
);
287+
continue;
288+
}
289+
290+
if (ownedCommands.length > 1) {
291+
log?.(
292+
`mattermost: multiple owned commands found for /${spec.trigger}; using the first and leaving extras untouched`,
293+
);
294+
}
295+
296+
const existingCmd = ownedCommands[0];
266297

267298
// Already registered with the correct callback URL
268299
if (existingCmd && existingCmd.url === callbackUrl) {
@@ -307,7 +338,7 @@ export async function registerSlashCommands(params: {
307338
log?.(
308339
`mattermost: failed to update command /${spec.trigger} (id=${existingCmd.id}): ${String(err)}`,
309340
);
310-
// Fallback: try delete+recreate (safe for the oc_* namespace).
341+
// Fallback: try delete+recreate for commands owned by this bot user.
311342
try {
312343
await deleteMattermostCommand(client, existingCmd.id);
313344
log?.(`mattermost: deleted stale command /${spec.trigger} (id=${existingCmd.id})`);

‎extensions/mattermost/src/mattermost/slash-http.ts‎

Lines changed: 46 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type { OpenClawConfig, ReplyPayload, RuntimeEnv } from "openclaw/plugin-s
1010
import {
1111
createReplyPrefixOptions,
1212
createTypingCallbacks,
13+
isDangerousNameMatchingEnabled,
1314
logTypingFailure,
1415
resolveControlCommandGate,
1516
} from "openclaw/plugin-sdk";
@@ -23,6 +24,11 @@ import {
2324
sendMattermostTyping,
2425
type MattermostChannel,
2526
} from "./client.js";
27+
import {
28+
isMattermostSenderAllowed,
29+
normalizeMattermostAllowList,
30+
resolveMattermostEffectiveAllowFromLists,
31+
} from "./monitor-auth.js";
2632
import { sendMessageMattermost } from "./send.js";
2733
import {
2834
parseSlashCommandPayload,
@@ -72,46 +78,6 @@ function sendJsonResponse(
7278
res.end(JSON.stringify(body));
7379
}
7480

75-
/**
76-
* Normalize a single allowlist entry, matching the websocket monitor behaviour.
77-
* Strips `mattermost:`, `user:`, and `@` prefixes, and preserves the `*` wildcard.
78-
*/
79-
function normalizeAllowEntry(entry: string): string {
80-
const trimmed = entry.trim();
81-
if (!trimmed) {
82-
return "";
83-
}
84-
if (trimmed === "*") {
85-
return "*";
86-
}
87-
return trimmed
88-
.replace(/^(mattermost|user):/i, "")
89-
.replace(/^@/, "")
90-
.toLowerCase();
91-
}
92-
93-
function normalizeAllowList(entries: Array<string | number>): string[] {
94-
const normalized = entries.map((entry) => normalizeAllowEntry(String(entry))).filter(Boolean);
95-
return Array.from(new Set(normalized));
96-
}
97-
98-
function isSenderAllowed(params: { senderId: string; senderName: string; allowFrom: string[] }) {
99-
const { senderId, senderName, allowFrom } = params;
100-
if (allowFrom.length === 0) {
101-
return false;
102-
}
103-
if (allowFrom.includes("*")) {
104-
return true;
105-
}
106-
107-
const normalizedId = normalizeAllowEntry(senderId);
108-
const normalizedName = senderName ? normalizeAllowEntry(senderName) : "";
109-
110-
return allowFrom.some(
111-
(entry) => entry === normalizedId || (normalizedName && entry === normalizedName),
112-
);
113-
}
114-
11581
type SlashInvocationAuth = {
11682
ok: boolean;
11783
denyResponse?: MattermostSlashCommandResponse;
@@ -181,49 +147,58 @@ async function authorizeSlashInvocation(params: {
181147
const dmPolicy = account.config.dmPolicy ?? "pairing";
182148
const defaultGroupPolicy = cfg.channels?.defaults?.groupPolicy;
183149
const groupPolicy = account.config.groupPolicy ?? defaultGroupPolicy ?? "allowlist";
150+
const allowNameMatching = isDangerousNameMatchingEnabled(account.config);
184151

185-
const configAllowFrom = normalizeAllowList(account.config.allowFrom ?? []);
186-
const configGroupAllowFrom = normalizeAllowList(account.config.groupAllowFrom ?? []);
187-
const storeAllowFrom = normalizeAllowList(
152+
const configAllowFrom = normalizeMattermostAllowList(account.config.allowFrom ?? []);
153+
const configGroupAllowFrom = normalizeMattermostAllowList(account.config.groupAllowFrom ?? []);
154+
const storeAllowFrom = normalizeMattermostAllowList(
188155
await core.channel.pairing
189156
.readAllowFromStore({
190157
channel: "mattermost",
191158
accountId: account.accountId,
192159
})
193160
.catch(() => []),
194161
);
195-
const effectiveAllowFrom = Array.from(new Set([...configAllowFrom, ...storeAllowFrom]));
196-
const effectiveGroupAllowFrom = Array.from(
197-
new Set([
198-
...(configGroupAllowFrom.length > 0 ? configGroupAllowFrom : configAllowFrom),
199-
...storeAllowFrom,
200-
]),
201-
);
162+
const { effectiveAllowFrom, effectiveGroupAllowFrom } = resolveMattermostEffectiveAllowFromLists({
163+
allowFrom: configAllowFrom,
164+
groupAllowFrom: configGroupAllowFrom,
165+
storeAllowFrom,
166+
dmPolicy,
167+
});
202168

203169
const allowTextCommands = core.channel.commands.shouldHandleTextCommands({
204170
cfg,
205171
surface: "mattermost",
206172
});
207173
const hasControlCommand = core.channel.text.hasControlCommand(commandText, cfg);
208174
const useAccessGroups = cfg.commands?.useAccessGroups !== false;
175+
const commandDmAllowFrom = kind === "direct" ? effectiveAllowFrom : configAllowFrom;
176+
const commandGroupAllowFrom =
177+
kind === "direct"
178+
? effectiveGroupAllowFrom
179+
: configGroupAllowFrom.length > 0
180+
? configGroupAllowFrom
181+
: configAllowFrom;
209182

210-
const senderAllowedForCommands = isSenderAllowed({
183+
const senderAllowedForCommands = isMattermostSenderAllowed({
211184
senderId,
212185
senderName,
213-
allowFrom: effectiveAllowFrom,
186+
allowFrom: commandDmAllowFrom,
187+
allowNameMatching,
214188
});
215-
const groupAllowedForCommands = isSenderAllowed({
189+
const groupAllowedForCommands = isMattermostSenderAllowed({
216190
senderId,
217191
senderName,
218-
allowFrom: effectiveGroupAllowFrom,
192+
allowFrom: commandGroupAllowFrom,
193+
allowNameMatching,
219194
});
220195

221196
const commandGate = resolveControlCommandGate({
222197
useAccessGroups,
223198
authorizers: [
224-
{ configured: effectiveAllowFrom.length > 0, allowed: senderAllowedForCommands },
199+
{ configured: commandDmAllowFrom.length > 0, allowed: senderAllowedForCommands },
225200
{
226-
configured: effectiveGroupAllowFrom.length > 0,
201+
configured: commandGroupAllowFrom.length > 0,
227202
allowed: groupAllowedForCommands,
228203
},
229204
],
@@ -661,16 +636,22 @@ async function handleSlashCommandAsync(params: {
661636
onReplyStart: typingCallbacks.onReplyStart,
662637
});
663638

664-
await core.channel.reply.dispatchReplyFromConfig({
665-
ctx: ctxPayload,
666-
cfg,
639+
await core.channel.reply.withReplyDispatcher({
667640
dispatcher,
668-
replyOptions: {
669-
...replyOptions,
670-
disableBlockStreaming:
671-
typeof account.blockStreaming === "boolean" ? !account.blockStreaming : undefined,
672-
onModelSelected,
641+
onSettled: () => {
642+
markDispatchIdle();
673643
},
644+
run: () =>
645+
core.channel.reply.dispatchReplyFromConfig({
646+
ctx: ctxPayload,
647+
cfg,
648+
dispatcher,
649+
replyOptions: {
650+
...replyOptions,
651+
disableBlockStreaming:
652+
typeof account.blockStreaming === "boolean" ? !account.blockStreaming : undefined,
653+
onModelSelected,
654+
},
655+
}),
674656
});
675-
markDispatchIdle();
676657
}

0 commit comments

Comments
 (0)