From 4a80d48ea9c4c1377c163cdf45b484cb6aebbc02 Mon Sep 17 00:00:00 2001 From: Muhammed Mukhthar CM <56378562+mukhtharcm@users.noreply.github.com> Date: Fri, 6 Mar 2026 15:27:47 +0530 Subject: [PATCH] fix(mattermost): allow reachable interaction callback URLs (#37543) Merged via squash. Prepared head SHA: 4d593731be5a5dcbf3106d596b38acfeb8cf0aa8 Co-authored-by: mukhtharcm <56378562+mukhtharcm@users.noreply.github.com> Co-authored-by: mukhtharcm <56378562+mukhtharcm@users.noreply.github.com> Reviewed-by: @mukhtharcm --- CHANGELOG.md | 1 + docs/channels/mattermost.md | 13 +- extensions/mattermost/src/channel.ts | 5 +- .../src/mattermost/interactions.test.ts | 248 +++++++++++++++--- .../mattermost/src/mattermost/interactions.ts | 86 ++++-- .../mattermost/src/mattermost/monitor.ts | 33 ++- 6 files changed, 316 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1624ce8196d..5cb86a8bd78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -194,6 +194,7 @@ Docs: https://docs.openclaw.ai - Agents/reply delivery timing: flush embedded Pi block replies before waiting on compaction retries so already-generated assistant replies reach channels before compaction wait completes. (#35489) thanks @Sid-Qin. - Agents/gateway config guidance: stop exposing `config.schema` through the agent `gateway` tool, remove prompt/docs guidance that told agents to call it, and keep agents on `config.get` plus `config.patch`/`config.apply` for config changes. (#7382) thanks @kakuteki. - Agents/failover: classify periodic provider limit exhaustion text (for example `Weekly/Monthly Limit Exhausted`) as `rate_limit` while keeping explicit `402 Payment Required` variants in billing, so failover continues without misclassifying billing-wrapped quota errors. (#33813) thanks @zhouhe-xydt. +- Mattermost/interactive button callbacks: allow external callback base URLs and stop requiring loopback-origin requests so button clicks work when Mattermost reaches the gateway over Tailscale, LAN, or a reverse proxy. (#37543) thanks @mukhtharcm. ## 2026.3.2 diff --git a/docs/channels/mattermost.md b/docs/channels/mattermost.md index fdfd48a4dbf..f9417109a77 100644 --- a/docs/channels/mattermost.md +++ b/docs/channels/mattermost.md @@ -221,6 +221,17 @@ Config: - `channels.mattermost.capabilities`: array of capability strings. Add `"inlineButtons"` to enable the buttons tool description in the agent system prompt. +- `channels.mattermost.interactions.callbackBaseUrl`: optional external base URL for button + callbacks (for example `https://gateway.example.com`). Use this when Mattermost cannot + reach the gateway at its bind host directly. +- In multi-account setups, you can also set the same field under + `channels.mattermost.accounts..interactions.callbackBaseUrl`. +- If `interactions.callbackBaseUrl` is omitted, OpenClaw derives the callback URL from + `gateway.customBindHost` + `gateway.port`, then falls back to `http://localhost:`. +- Reachability rule: the button callback URL must be reachable from the Mattermost server. + `localhost` only works when Mattermost and OpenClaw run on the same host/network namespace. +- If your callback target is private/tailnet/internal, add its host/domain to Mattermost + `ServiceSettings.AllowedUntrustedInternalConnections`. ### Direct API integration (external scripts) @@ -244,7 +255,7 @@ the extension when possible; if posting raw JSON, follow these rules: name: "Approve", // display label style: "primary", // optional: "default", "primary", "danger" integration: { - url: "http://localhost:18789/mattermost/interactions/default", + url: "https://gateway.example.com/mattermost/interactions/default", context: { action_id: "mybutton01", // must match button id (for name lookup) action: "approve", diff --git a/extensions/mattermost/src/channel.ts b/extensions/mattermost/src/channel.ts index 5897c11277a..00e4c69e0f7 100644 --- a/extensions/mattermost/src/channel.ts +++ b/extensions/mattermost/src/channel.ts @@ -165,7 +165,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 callbackUrl = resolveInteractionCallbackUrl(account.accountId, cfg); + const callbackUrl = resolveInteractionCallbackUrl(account.accountId, { + gateway: cfg.gateway, + interactions: account.config.interactions, + }); // Flatten 2D array (rows of buttons) to 1D — core schema sends Array> // but Mattermost doesn't have row layout, so we flatten all rows into a single list. diff --git a/extensions/mattermost/src/mattermost/interactions.test.ts b/extensions/mattermost/src/mattermost/interactions.test.ts index 0e24ae4a4ee..19d39676a27 100644 --- a/extensions/mattermost/src/mattermost/interactions.test.ts +++ b/extensions/mattermost/src/mattermost/interactions.test.ts @@ -1,11 +1,16 @@ -import { type IncomingMessage } from "node:http"; +import { type IncomingMessage, type ServerResponse } from "node:http"; import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { setMattermostRuntime } from "../runtime.js"; +import { resolveMattermostAccount } from "./accounts.js"; +import type { MattermostClient } from "./client.js"; import { buildButtonAttachments, + computeInteractionCallbackUrl, + createMattermostInteractionHandler, generateInteractionToken, getInteractionCallbackUrl, getInteractionSecret, - isLocalhostRequest, + resolveInteractionCallbackPath, resolveInteractionCallbackUrl, setInteractionCallbackUrl, setInteractionSecret, @@ -132,7 +137,9 @@ describe("callback URL registry", () => { describe("resolveInteractionCallbackUrl", () => { afterEach(() => { - setInteractionCallbackUrl("resolve-test", ""); + for (const accountId of ["cached", "default", "acct", "myaccount"]) { + setInteractionCallbackUrl(accountId, ""); + } }); it("prefers cached URL from registry", () => { @@ -140,19 +147,99 @@ describe("resolveInteractionCallbackUrl", () => { expect(resolveInteractionCallbackUrl("cached")).toBe("http://cached:1234/path"); }); - it("falls back to computed URL from gateway port config", () => { - const url = resolveInteractionCallbackUrl("default", { gateway: { port: 9999 } }); + it("recomputes from config when bypassing the cache explicitly", () => { + setInteractionCallbackUrl("acct", "http://cached:1234/path"); + const url = computeInteractionCallbackUrl("acct", { + gateway: { port: 9999, customBindHost: "gateway.internal" }, + }); + expect(url).toBe("http://gateway.internal:9999/mattermost/interactions/acct"); + }); + + it("uses interactions.callbackBaseUrl when configured", () => { + const url = resolveInteractionCallbackUrl("default", { + channels: { + mattermost: { + interactions: { + callbackBaseUrl: "https://gateway.example.com/openclaw", + }, + }, + }, + }); + expect(url).toBe("https://gateway.example.com/openclaw/mattermost/interactions/default"); + }); + + it("trims trailing slashes from callbackBaseUrl", () => { + const url = resolveInteractionCallbackUrl("acct", { + channels: { + mattermost: { + interactions: { + callbackBaseUrl: "https://gateway.example.com/root///", + }, + }, + }, + }); + expect(url).toBe("https://gateway.example.com/root/mattermost/interactions/acct"); + }); + + it("uses merged per-account interactions.callbackBaseUrl", () => { + const cfg = { + gateway: { port: 9999 }, + channels: { + mattermost: { + accounts: { + acct: { + botToken: "bot-token", + baseUrl: "https://chat.example.com", + interactions: { + callbackBaseUrl: "https://gateway.example.com/root", + }, + }, + }, + }, + }, + }; + const account = resolveMattermostAccount({ + cfg, + accountId: "acct", + allowUnresolvedSecretRef: true, + }); + const url = resolveInteractionCallbackUrl(account.accountId, { + gateway: cfg.gateway, + interactions: account.config.interactions, + }); + expect(url).toBe("https://gateway.example.com/root/mattermost/interactions/acct"); + }); + + it("falls back to gateway.customBindHost when configured", () => { + const url = resolveInteractionCallbackUrl("default", { + gateway: { port: 9999, customBindHost: "gateway.internal" }, + }); + expect(url).toBe("http://gateway.internal:9999/mattermost/interactions/default"); + }); + + it("falls back to localhost when customBindHost is a wildcard bind address", () => { + const url = resolveInteractionCallbackUrl("default", { + gateway: { port: 9999, customBindHost: "0.0.0.0" }, + }); expect(url).toBe("http://localhost:9999/mattermost/interactions/default"); }); + it("brackets IPv6 custom bind hosts", () => { + const url = resolveInteractionCallbackUrl("acct", { + gateway: { port: 9999, customBindHost: "::1" }, + }); + expect(url).toBe("http://[::1]:9999/mattermost/interactions/acct"); + }); + it("uses default port 18789 when no config provided", () => { const url = resolveInteractionCallbackUrl("myaccount"); expect(url).toBe("http://localhost:18789/mattermost/interactions/myaccount"); }); +}); - it("uses default port when gateway config has no port", () => { - const url = resolveInteractionCallbackUrl("acct", { gateway: {} }); - expect(url).toBe("http://localhost:18789/mattermost/interactions/acct"); +describe("resolveInteractionCallbackPath", () => { + it("builds the per-account callback path", () => { + expect(resolveInteractionCallbackPath("acct")).toBe("/mattermost/interactions/acct"); }); }); @@ -299,37 +386,134 @@ describe("buildButtonAttachments", () => { }); }); -// ── isLocalhostRequest ─────────────────────────────────────────────── +describe("createMattermostInteractionHandler", () => { + beforeEach(() => { + setMattermostRuntime({ + system: { + enqueueSystemEvent: () => {}, + }, + } as unknown as Parameters[0]); + setInteractionSecret("acct", "bot-token"); + }); -describe("isLocalhostRequest", () => { - function fakeReq(remoteAddress?: string): IncomingMessage { - return { - socket: { remoteAddress }, - } as unknown as IncomingMessage; + function createReq(params: { + method?: string; + body?: unknown; + remoteAddress?: string; + }): IncomingMessage { + const body = params.body === undefined ? "" : JSON.stringify(params.body); + const listeners = new Map void>>(); + + const req = { + method: params.method ?? "POST", + socket: { remoteAddress: params.remoteAddress ?? "203.0.113.10" }, + on(event: string, handler: (...args: unknown[]) => void) { + const existing = listeners.get(event) ?? []; + existing.push(handler); + listeners.set(event, existing); + return this; + }, + } as IncomingMessage & { emitTest: (event: string, ...args: unknown[]) => void }; + + req.emitTest = (event: string, ...args: unknown[]) => { + const handlers = listeners.get(event) ?? []; + for (const handler of handlers) { + handler(...args); + } + }; + + queueMicrotask(() => { + if (body) { + req.emitTest("data", Buffer.from(body)); + } + req.emitTest("end"); + }); + + return req; } - it("accepts 127.0.0.1", () => { - expect(isLocalhostRequest(fakeReq("127.0.0.1"))).toBe(true); + function createRes(): ServerResponse & { headers: Record; body: string } { + const res = { + statusCode: 200, + headers: {} as Record, + body: "", + setHeader(name: string, value: string) { + res.headers[name] = value; + }, + end(chunk?: string) { + res.body = chunk ?? ""; + }, + }; + return res as unknown as ServerResponse & { headers: Record; body: string }; + } + + it("accepts non-localhost requests when the interaction token is valid", async () => { + const context = { action_id: "approve" }; + const token = generateInteractionToken(context, "acct"); + const requestLog: Array<{ path: string; method?: string }> = []; + const handler = createMattermostInteractionHandler({ + client: { + request: async (path: string, init?: { method?: string }) => { + requestLog.push({ path, method: init?.method }); + if (init?.method === "PUT") { + return { id: "post-1" }; + } + return { + message: "Choose", + props: { + attachments: [{ actions: [{ id: "approve", name: "Approve" }] }], + }, + }; + }, + } as unknown as MattermostClient, + botUserId: "bot", + accountId: "acct", + }); + + const req = createReq({ + remoteAddress: "198.51.100.8", + body: { + user_id: "user-1", + user_name: "alice", + channel_id: "chan-1", + post_id: "post-1", + context: { ...context, _token: token }, + }, + }); + const res = createRes(); + + await handler(req, res); + + expect(res.statusCode).toBe(200); + expect(res.body).toBe("{}"); + expect(requestLog).toEqual([ + { path: "/posts/post-1", method: undefined }, + { path: "/posts/post-1", method: "PUT" }, + ]); }); - it("accepts ::1", () => { - expect(isLocalhostRequest(fakeReq("::1"))).toBe(true); - }); + it("rejects requests with an invalid interaction token", async () => { + const handler = createMattermostInteractionHandler({ + client: { + request: async () => ({ message: "unused" }), + } as unknown as MattermostClient, + botUserId: "bot", + accountId: "acct", + }); - it("accepts ::ffff:127.0.0.1", () => { - expect(isLocalhostRequest(fakeReq("::ffff:127.0.0.1"))).toBe(true); - }); + const req = createReq({ + body: { + user_id: "user-1", + channel_id: "chan-1", + post_id: "post-1", + context: { action_id: "approve", _token: "deadbeef" }, + }, + }); + const res = createRes(); - it("rejects external addresses", () => { - expect(isLocalhostRequest(fakeReq("10.0.0.1"))).toBe(false); - expect(isLocalhostRequest(fakeReq("192.168.1.1"))).toBe(false); - }); + await handler(req, res); - it("rejects when socket has no remote address", () => { - expect(isLocalhostRequest(fakeReq(undefined))).toBe(false); - }); - - it("rejects when socket is missing", () => { - expect(isLocalhostRequest({} as IncomingMessage)).toBe(false); + expect(res.statusCode).toBe(403); + expect(res.body).toContain("Invalid token"); }); }); diff --git a/extensions/mattermost/src/mattermost/interactions.ts b/extensions/mattermost/src/mattermost/interactions.ts index be305db4ba3..33415ae519c 100644 --- a/extensions/mattermost/src/mattermost/interactions.ts +++ b/extensions/mattermost/src/mattermost/interactions.ts @@ -1,5 +1,6 @@ import { createHmac, timingSafeEqual } from "node:crypto"; import type { IncomingMessage, ServerResponse } from "node:http"; +import type { OpenClawConfig } from "openclaw/plugin-sdk/mattermost"; import { getMattermostRuntime } from "../runtime.js"; import { updateMattermostPost, type MattermostClient } from "./client.js"; @@ -43,21 +44,72 @@ export function getInteractionCallbackUrl(accountId: string): string | undefined return callbackUrls.get(accountId); } +type InteractionCallbackConfig = Pick & { + interactions?: { + callbackBaseUrl?: string; + }; +}; + +export function resolveInteractionCallbackPath(accountId: string): string { + return `/mattermost/interactions/${accountId}`; +} + +function isWildcardBindHost(rawHost: string): boolean { + const trimmed = rawHost.trim(); + if (!trimmed) return false; + const host = trimmed.startsWith("[") && trimmed.endsWith("]") ? trimmed.slice(1, -1) : trimmed; + return host === "0.0.0.0" || host === "::" || host === "0:0:0:0:0:0:0:0" || host === "::0"; +} + +function normalizeCallbackBaseUrl(baseUrl: string): string { + return baseUrl.trim().replace(/\/+$/, ""); +} + /** * Resolve the interaction callback URL for an account. - * Prefers the in-memory registered URL (set by the gateway monitor). - * Falls back to computing it from the gateway port in config (for CLI callers). + * Falls back to computing it from interactions.callbackBaseUrl or gateway host config. + */ +export function computeInteractionCallbackUrl( + accountId: string, + cfg?: InteractionCallbackConfig, +): string { + const path = resolveInteractionCallbackPath(accountId); + // Prefer merged per-account config when available, but keep the top-level path for + // callers/tests that still pass the root Mattermost config shape directly. + const callbackBaseUrl = + cfg?.interactions?.callbackBaseUrl?.trim() ?? + cfg?.channels?.mattermost?.interactions?.callbackBaseUrl?.trim(); + if (callbackBaseUrl) { + return `${normalizeCallbackBaseUrl(callbackBaseUrl)}${path}`; + } + const port = typeof cfg?.gateway?.port === "number" ? cfg.gateway.port : 18789; + let host = + cfg?.gateway?.customBindHost && !isWildcardBindHost(cfg.gateway.customBindHost) + ? cfg.gateway.customBindHost.trim() + : "localhost"; + + // Bracket IPv6 literals so the URL is valid: http://[::1]:18789/... + if (host.includes(":") && !(host.startsWith("[") && host.endsWith("]"))) { + host = `[${host}]`; + } + + return `http://${host}:${port}${path}`; +} + +/** + * Resolve the interaction callback URL for an account. + * Prefers the in-memory registered URL (set by the gateway monitor) so callers outside the + * monitor lifecycle can reuse the runtime-validated callback destination. */ export function resolveInteractionCallbackUrl( accountId: string, - cfg?: { gateway?: { port?: number } }, + cfg?: InteractionCallbackConfig, ): string { const cached = callbackUrls.get(accountId); if (cached) { return cached; } - const port = typeof cfg?.gateway?.port === "number" ? cfg.gateway.port : 18789; - return `http://localhost:${port}/mattermost/interactions/${accountId}`; + return computeInteractionCallbackUrl(accountId, cfg); } // ── HMAC token management ────────────────────────────────────────────── @@ -198,18 +250,6 @@ export function buildButtonAttachments(params: { ]; } -// ── Localhost validation ─────────────────────────────────────────────── - -const LOCALHOST_ADDRESSES = new Set(["127.0.0.1", "::1", "::ffff:127.0.0.1"]); - -export function isLocalhostRequest(req: IncomingMessage): boolean { - const addr = req.socket?.remoteAddress; - if (!addr) { - return false; - } - return LOCALHOST_ADDRESSES.has(addr); -} - // ── Request body reader ──────────────────────────────────────────────── function readInteractionBody(req: IncomingMessage): Promise { @@ -251,7 +291,6 @@ export function createMattermostInteractionHandler(params: { client: MattermostClient; botUserId: string; accountId: string; - callbackUrl: string; resolveSessionKey?: (channelId: string, userId: string) => Promise; dispatchButtonClick?: (opts: { channelId: string; @@ -276,17 +315,6 @@ export function createMattermostInteractionHandler(params: { return; } - // Verify request is from localhost - if (!isLocalhostRequest(req)) { - log?.( - `mattermost interaction: rejected non-localhost request from ${req.socket?.remoteAddress}`, - ); - res.statusCode = 403; - res.setHeader("Content-Type", "application/json"); - res.end(JSON.stringify({ error: "Forbidden" })); - return; - } - let payload: MattermostInteractionPayload; try { const raw = await readInteractionBody(req); diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 13864a33f44..4ce11a6a003 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -44,7 +44,9 @@ import { type MattermostUser, } from "./client.js"; import { + computeInteractionCallbackUrl, createMattermostInteractionHandler, + resolveInteractionCallbackPath, setInteractionCallbackUrl, setInteractionSecret, } from "./interactions.js"; @@ -100,6 +102,10 @@ const RECENT_MATTERMOST_MESSAGE_MAX = 2000; const CHANNEL_CACHE_TTL_MS = 5 * 60_000; const USER_CACHE_TTL_MS = 10 * 60_000; +function isLoopbackHost(hostname: string): boolean { + return hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1"; +} + const recentInboundMessages = createDedupeCache({ ttlMs: RECENT_MATTERMOST_MESSAGE_TTL_MS, maxSize: RECENT_MATTERMOST_MESSAGE_MAX, @@ -333,9 +339,6 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} gatewayHost: cfg.gateway?.customBindHost ?? undefined, }); - const isLoopbackHost = (hostname: string) => - hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1"; - try { const mmHost = new URL(baseUrl).hostname; const callbackHost = new URL(slashCallbackUrl).hostname; @@ -452,10 +455,27 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} // Register HTTP callback endpoint for interactive button clicks. // Mattermost POSTs to this URL when a user clicks a button action. - const gatewayPort = typeof cfg.gateway?.port === "number" ? cfg.gateway.port : 18789; - const interactionPath = `/mattermost/interactions/${account.accountId}`; - const callbackUrl = `http://localhost:${gatewayPort}${interactionPath}`; + const interactionPath = resolveInteractionCallbackPath(account.accountId); + // Recompute from config on each monitor start so reconnects or config reloads can refresh the + // cached callback URL for downstream callers such as `message action=send`. + const callbackUrl = computeInteractionCallbackUrl(account.accountId, { + gateway: cfg.gateway, + interactions: account.config.interactions, + }); setInteractionCallbackUrl(account.accountId, callbackUrl); + + try { + const mmHost = new URL(baseUrl).hostname; + const callbackHost = new URL(callbackUrl).hostname; + if (isLoopbackHost(callbackHost) && !isLoopbackHost(mmHost)) { + runtime.error?.( + `mattermost: interactions callbackUrl resolved to ${callbackUrl} (loopback) while baseUrl is ${baseUrl}. This MAY be unreachable depending on your deployment. If button clicks don't work, set channels.mattermost.interactions.callbackBaseUrl to a URL reachable from the Mattermost server (e.g. your public reverse proxy URL).`, + ); + } + } catch { + // URL parse failed; ignore and continue (we will fail naturally if callbacks cannot be delivered). + } + const unregisterInteractions = registerPluginHttpRoute({ path: interactionPath, fallbackPath: "/mattermost/interactions/default", @@ -464,7 +484,6 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} client, botUserId, accountId: account.accountId, - callbackUrl, resolveSessionKey: async (channelId: string, userId: string) => { const channelInfo = await resolveChannelInfo(channelId); const kind = mapMattermostChannelTypeToChatType(channelInfo?.type);