Skip to content

Commit 6d0547d

Browse files
teconomixmukhtharcm
teconomix
and
mukhtharcm
authored
mattermost: fix DM media upload for unprefixed user IDs (#29925)
Merged via squash. Prepared head SHA: 5cffcb0 Co-authored-by: teconomix <[email protected]> Co-authored-by: mukhtharcm <[email protected]> Reviewed-by: @mukhtharcm
1 parent 568b0a2 commit 6d0547d

File tree

15 files changed

+495
-12
lines changed

15 files changed

+495
-12
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,7 @@ Docs: https://docs.openclaw.ai
480480
- Agents/failover 402 recovery: keep temporary spend-limit `402` payloads retryable, preserve explicit insufficient-credit billing detection even in long provider payloads, and allow throttled billing-cooldown probes so single-provider setups can recover instead of staying locked out. (#38533) Thanks @xialonglee.
481481
- Browser/config schema: accept `browser.profiles.*.driver: "openclaw"` while preserving legacy `"clawd"` compatibility in validated config. (#39374; based on #35621) Thanks @gambletan and @ingyukoh.
482482
- Memory flush/bootstrap file protection: restrict memory-flush runs to append-only `read`/`write` tools and route host-side memory appends through root-enforced safe file handles so flush turns cannot overwrite bootstrap files via `exec` or unsafe raw rewrites. (#38574) Thanks @frankekn.
483+
- Mattermost/DM media uploads: resolve bare 26-character Mattermost IDs user-first for direct messages so media sends no longer fail with `403 Forbidden` when targets are configured as unprefixed user IDs. (#29925) Thanks @teconomix.
483484

484485
## 2026.3.2
485486

‎docs/automation/cron-jobs.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ If `delivery.channel` or `delivery.to` is omitted, cron can fall back to the mai
262262
Target format reminders:
263263

264264
- Slack/Discord/Mattermost (plugin) targets should use explicit prefixes (e.g. `channel:<id>`, `user:<id>`) to avoid ambiguity.
265+
Mattermost bare 26-char IDs are resolved **user-first** (DM if user exists, channel otherwise) — use `user:<id>` or `channel:<id>` for deterministic routing.
265266
- Telegram topics should use the `:topic:` form (see below).
266267

267268
#### Telegram delivery targets (topics / forum threads)

‎docs/channels/mattermost.md‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,14 @@ Use these target formats with `openclaw message send` or cron/webhooks:
153153
- `user:<id>` for a DM
154154
- `@username` for a DM (resolved via the Mattermost API)
155155

156-
Bare IDs are treated as channels.
156+
Bare opaque IDs (like `64ifufp...`) are **ambiguous** in Mattermost (user ID vs channel ID).
157+
158+
OpenClaw resolves them **user-first**:
159+
160+
- If the ID exists as a user (`GET /api/v4/users/<id>` succeeds), OpenClaw sends a **DM** by resolving the direct channel via `/api/v4/channels/direct`.
161+
- Otherwise the ID is treated as a **channel ID**.
162+
163+
If you need deterministic behavior, always use the explicit prefixes (`user:<id>` / `channel:<id>`).
157164

158165
## Reactions (message tool)
159166

‎extensions/mattermost/src/channel.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import { monitorMattermostProvider } from "./mattermost/monitor.js";
3535
import { probeMattermost } from "./mattermost/probe.js";
3636
import { addMattermostReaction, removeMattermostReaction } from "./mattermost/reactions.js";
3737
import { sendMessageMattermost } from "./mattermost/send.js";
38+
import { resolveMattermostOpaqueTarget } from "./mattermost/target-resolution.js";
3839
import { looksLikeMattermostTargetId, normalizeMattermostMessagingTarget } from "./normalize.js";
3940
import { mattermostOnboardingAdapter } from "./onboarding.js";
4041
import { getMattermostRuntime } from "./runtime.js";
@@ -340,6 +341,21 @@ export const mattermostPlugin: ChannelPlugin<ResolvedMattermostAccount> = {
340341
targetResolver: {
341342
looksLikeId: looksLikeMattermostTargetId,
342343
hint: "<channelId|user:ID|channel:ID>",
344+
resolveTarget: async ({ cfg, accountId, input }) => {
345+
const resolved = await resolveMattermostOpaqueTarget({
346+
input,
347+
cfg,
348+
accountId,
349+
});
350+
if (!resolved) {
351+
return null;
352+
}
353+
return {
354+
to: resolved.to,
355+
kind: resolved.kind,
356+
source: "directory",
357+
};
358+
},
343359
},
344360
},
345361
outbound: {

‎extensions/mattermost/src/mattermost/send.test.ts‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { beforeEach, describe, expect, it, vi } from "vitest";
22
import { parseMattermostTarget, sendMessageMattermost } from "./send.js";
3+
import { resetMattermostOpaqueTargetCacheForTests } from "./target-resolution.js";
34

45
const mockState = vi.hoisted(() => ({
56
loadConfig: vi.fn(() => ({})),
@@ -14,6 +15,7 @@ const mockState = vi.hoisted(() => ({
1415
createMattermostPost: vi.fn(),
1516
fetchMattermostChannelByName: vi.fn(),
1617
fetchMattermostMe: vi.fn(),
18+
fetchMattermostUser: vi.fn(),
1719
fetchMattermostUserTeams: vi.fn(),
1820
fetchMattermostUserByUsername: vi.fn(),
1921
normalizeMattermostBaseUrl: vi.fn((input: string | undefined) => input?.trim() ?? ""),
@@ -34,6 +36,7 @@ vi.mock("./client.js", () => ({
3436
createMattermostPost: mockState.createMattermostPost,
3537
fetchMattermostChannelByName: mockState.fetchMattermostChannelByName,
3638
fetchMattermostMe: mockState.fetchMattermostMe,
39+
fetchMattermostUser: mockState.fetchMattermostUser,
3740
fetchMattermostUserTeams: mockState.fetchMattermostUserTeams,
3841
fetchMattermostUserByUsername: mockState.fetchMattermostUserByUsername,
3942
normalizeMattermostBaseUrl: mockState.normalizeMattermostBaseUrl,
@@ -77,9 +80,11 @@ describe("sendMessageMattermost", () => {
7780
mockState.createMattermostPost.mockReset();
7881
mockState.fetchMattermostChannelByName.mockReset();
7982
mockState.fetchMattermostMe.mockReset();
83+
mockState.fetchMattermostUser.mockReset();
8084
mockState.fetchMattermostUserTeams.mockReset();
8185
mockState.fetchMattermostUserByUsername.mockReset();
8286
mockState.uploadMattermostFile.mockReset();
87+
resetMattermostOpaqueTargetCacheForTests();
8388
mockState.createMattermostClient.mockReturnValue({});
8489
mockState.createMattermostPost.mockResolvedValue({ id: "post-1" });
8590
mockState.fetchMattermostMe.mockResolvedValue({ id: "bot-user" });
@@ -182,6 +187,61 @@ describe("sendMessageMattermost", () => {
182187
}),
183188
);
184189
});
190+
191+
it("resolves a bare Mattermost user id as a DM target before upload", async () => {
192+
const userId = "dthcxgoxhifn3pwh65cut3ud3w";
193+
mockState.fetchMattermostUser.mockResolvedValueOnce({ id: userId });
194+
mockState.createMattermostDirectChannel.mockResolvedValueOnce({ id: "dm-channel-1" });
195+
mockState.loadOutboundMediaFromUrl.mockResolvedValueOnce({
196+
buffer: Buffer.from("media-bytes"),
197+
fileName: "photo.png",
198+
contentType: "image/png",
199+
kind: "image",
200+
});
201+
202+
const result = await sendMessageMattermost(userId, "hello", {
203+
mediaUrl: "file:///tmp/agent-workspace/photo.png",
204+
mediaLocalRoots: ["/tmp/agent-workspace"],
205+
});
206+
207+
expect(mockState.fetchMattermostUser).toHaveBeenCalledWith({}, userId);
208+
expect(mockState.createMattermostDirectChannel).toHaveBeenCalledWith({}, ["bot-user", userId]);
209+
expect(mockState.uploadMattermostFile).toHaveBeenCalledWith(
210+
{},
211+
expect.objectContaining({
212+
channelId: "dm-channel-1",
213+
}),
214+
);
215+
expect(result.channelId).toBe("dm-channel-1");
216+
});
217+
218+
it("falls back to a channel target when bare Mattermost id is not a user", async () => {
219+
const channelId = "aaaaaaaaaaaaaaaaaaaaaaaaaa";
220+
mockState.fetchMattermostUser.mockRejectedValueOnce(
221+
new Error("Mattermost API 404 Not Found: user not found"),
222+
);
223+
mockState.loadOutboundMediaFromUrl.mockResolvedValueOnce({
224+
buffer: Buffer.from("media-bytes"),
225+
fileName: "photo.png",
226+
contentType: "image/png",
227+
kind: "image",
228+
});
229+
230+
const result = await sendMessageMattermost(channelId, "hello", {
231+
mediaUrl: "file:///tmp/agent-workspace/photo.png",
232+
mediaLocalRoots: ["/tmp/agent-workspace"],
233+
});
234+
235+
expect(mockState.fetchMattermostUser).toHaveBeenCalledWith({}, channelId);
236+
expect(mockState.createMattermostDirectChannel).not.toHaveBeenCalled();
237+
expect(mockState.uploadMattermostFile).toHaveBeenCalledWith(
238+
{},
239+
expect.objectContaining({
240+
channelId,
241+
}),
242+
);
243+
expect(result.channelId).toBe(channelId);
244+
});
185245
});
186246

187247
describe("parseMattermostTarget", () => {
@@ -266,3 +326,110 @@ describe("parseMattermostTarget", () => {
266326
expect(parseMattermostTarget("Mattermost:QRS")).toEqual({ kind: "user", id: "QRS" });
267327
});
268328
});
329+
330+
// Each test uses a unique (token, id) pair to avoid module-level cache collisions.
331+
// userIdResolutionCache and dmChannelCache are module singletons that survive across tests.
332+
// Using unique cache keys per test ensures full isolation without needing a cache reset API.
333+
describe("sendMessageMattermost user-first resolution", () => {
334+
function makeAccount(token: string) {
335+
return {
336+
accountId: "default",
337+
botToken: token,
338+
baseUrl: "https://mattermost.example.com",
339+
};
340+
}
341+
342+
beforeEach(() => {
343+
vi.clearAllMocks();
344+
mockState.createMattermostClient.mockReturnValue({});
345+
mockState.createMattermostPost.mockResolvedValue({ id: "post-id" });
346+
mockState.createMattermostDirectChannel.mockResolvedValue({ id: "dm-channel-id" });
347+
mockState.fetchMattermostMe.mockResolvedValue({ id: "bot-id" });
348+
});
349+
350+
it("resolves unprefixed 26-char id as user and sends via DM channel", async () => {
351+
// Unique token + id to avoid cache pollution from other tests
352+
const userId = "aaaaaa1111111111aaaaaa1111"; // 26 chars
353+
mockState.resolveMattermostAccount.mockReturnValue(makeAccount("token-user-dm-t1"));
354+
mockState.fetchMattermostUser.mockResolvedValueOnce({ id: userId });
355+
356+
const res = await sendMessageMattermost(userId, "hello");
357+
358+
expect(mockState.fetchMattermostUser).toHaveBeenCalledTimes(1);
359+
expect(mockState.createMattermostDirectChannel).toHaveBeenCalledTimes(1);
360+
const params = mockState.createMattermostPost.mock.calls[0]?.[1];
361+
expect(params.channelId).toBe("dm-channel-id");
362+
expect(res.channelId).toBe("dm-channel-id");
363+
expect(res.messageId).toBe("post-id");
364+
});
365+
366+
it("falls back to channel id when user lookup returns 404", async () => {
367+
// Unique token + id for this test
368+
const channelId = "bbbbbb2222222222bbbbbb2222"; // 26 chars
369+
mockState.resolveMattermostAccount.mockReturnValue(makeAccount("token-404-t2"));
370+
const err = new Error("Mattermost API 404: user not found");
371+
mockState.fetchMattermostUser.mockRejectedValueOnce(err);
372+
373+
const res = await sendMessageMattermost(channelId, "hello");
374+
375+
expect(mockState.fetchMattermostUser).toHaveBeenCalledTimes(1);
376+
expect(mockState.createMattermostDirectChannel).not.toHaveBeenCalled();
377+
const params = mockState.createMattermostPost.mock.calls[0]?.[1];
378+
expect(params.channelId).toBe(channelId);
379+
expect(res.channelId).toBe(channelId);
380+
});
381+
382+
it("falls back to channel id without caching negative result on transient error", async () => {
383+
// Two unique tokens so each call has its own cache namespace
384+
const userId = "cccccc3333333333cccccc3333"; // 26 chars
385+
const tokenA = "token-transient-t3a";
386+
const tokenB = "token-transient-t3b";
387+
const transientErr = new Error("Mattermost API 503: service unavailable");
388+
389+
// First call: transient error → fall back to channel id, do NOT cache negative
390+
mockState.resolveMattermostAccount.mockReturnValue(makeAccount(tokenA));
391+
mockState.fetchMattermostUser.mockRejectedValueOnce(transientErr);
392+
393+
const res1 = await sendMessageMattermost(userId, "first");
394+
expect(res1.channelId).toBe(userId);
395+
396+
// Second call with a different token (new cache key) → retries user lookup
397+
vi.clearAllMocks();
398+
mockState.createMattermostClient.mockReturnValue({});
399+
mockState.createMattermostPost.mockResolvedValue({ id: "post-id-2" });
400+
mockState.createMattermostDirectChannel.mockResolvedValue({ id: "dm-channel-id" });
401+
mockState.fetchMattermostMe.mockResolvedValue({ id: "bot-id" });
402+
mockState.resolveMattermostAccount.mockReturnValue(makeAccount(tokenB));
403+
mockState.fetchMattermostUser.mockResolvedValueOnce({ id: userId });
404+
405+
const res2 = await sendMessageMattermost(userId, "second");
406+
expect(mockState.fetchMattermostUser).toHaveBeenCalledTimes(1);
407+
expect(res2.channelId).toBe("dm-channel-id");
408+
});
409+
410+
it("does not apply user-first resolution for explicit user: prefix", async () => {
411+
// Unique token + id — explicit user: prefix bypasses probe, goes straight to DM
412+
const userId = "dddddd4444444444dddddd4444"; // 26 chars
413+
mockState.resolveMattermostAccount.mockReturnValue(makeAccount("token-explicit-user-t4"));
414+
415+
const res = await sendMessageMattermost(`user:${userId}`, "hello");
416+
417+
expect(mockState.fetchMattermostUser).not.toHaveBeenCalled();
418+
expect(mockState.createMattermostDirectChannel).toHaveBeenCalledTimes(1);
419+
expect(res.channelId).toBe("dm-channel-id");
420+
});
421+
422+
it("does not apply user-first resolution for explicit channel: prefix", async () => {
423+
// Unique token + id — explicit channel: prefix, no probe, no DM
424+
const chanId = "eeeeee5555555555eeeeee5555"; // 26 chars
425+
mockState.resolveMattermostAccount.mockReturnValue(makeAccount("token-explicit-chan-t5"));
426+
427+
const res = await sendMessageMattermost(`channel:${chanId}`, "hello");
428+
429+
expect(mockState.fetchMattermostUser).not.toHaveBeenCalled();
430+
expect(mockState.createMattermostDirectChannel).not.toHaveBeenCalled();
431+
const params = mockState.createMattermostPost.mock.calls[0]?.[1];
432+
expect(params.channelId).toBe(chanId);
433+
expect(res.channelId).toBe(chanId);
434+
});
435+
});

‎extensions/mattermost/src/mattermost/send.ts‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
setInteractionSecret,
2020
type MattermostInteractiveButtonInput,
2121
} from "./interactions.js";
22+
import { isMattermostId, resolveMattermostOpaqueTarget } from "./target-resolution.js";
2223

2324
export type MattermostSendOpts = {
2425
cfg?: OpenClawConfig;
@@ -50,6 +51,7 @@ type MattermostTarget =
5051
const botUserCache = new Map<string, MattermostUser>();
5152
const userByNameCache = new Map<string, MattermostUser>();
5253
const channelByNameCache = new Map<string, string>();
54+
const dmChannelCache = new Map<string, string>();
5355

5456
const getCore = () => getMattermostRuntime();
5557

@@ -66,12 +68,6 @@ function normalizeMessage(text: string, mediaUrl?: string): string {
6668
function isHttpUrl(value: string): boolean {
6769
return /^https?:\/\//i.test(value);
6870
}
69-
70-
/** Mattermost IDs are 26-character lowercase alphanumeric strings. */
71-
function isMattermostId(value: string): boolean {
72-
return /^[a-z0-9]{26}$/.test(value);
73-
}
74-
7571
export function parseMattermostTarget(raw: string): MattermostTarget {
7672
const trimmed = raw.trim();
7773
if (!trimmed) {
@@ -208,12 +204,18 @@ async function resolveTargetChannelId(params: {
208204
token: params.token,
209205
username: params.target.username ?? "",
210206
});
207+
const dmKey = `${cacheKey(params.baseUrl, params.token)}::dm::${userId}`;
208+
const cachedDm = dmChannelCache.get(dmKey);
209+
if (cachedDm) {
210+
return cachedDm;
211+
}
211212
const botUser = await resolveBotUser(params.baseUrl, params.token);
212213
const client = createMattermostClient({
213214
baseUrl: params.baseUrl,
214215
botToken: params.token,
215216
});
216217
const channel = await createMattermostDirectChannel(client, [botUser.id, userId]);
218+
dmChannelCache.set(dmKey, channel.id);
217219
return channel.id;
218220
}
219221

@@ -248,7 +250,18 @@ async function resolveMattermostSendContext(
248250
);
249251
}
250252

251-
const target = parseMattermostTarget(to);
253+
const trimmedTo = to?.trim() ?? "";
254+
const opaqueTarget = await resolveMattermostOpaqueTarget({
255+
input: trimmedTo,
256+
token,
257+
baseUrl,
258+
});
259+
const target =
260+
opaqueTarget?.kind === "user"
261+
? { kind: "user" as const, id: opaqueTarget.id }
262+
: opaqueTarget?.kind === "channel"
263+
? { kind: "channel" as const, id: opaqueTarget.id }
264+
: parseMattermostTarget(trimmedTo);
252265
const channelId = await resolveTargetChannelId({
253266
target,
254267
baseUrl,

0 commit comments

Comments
 (0)