feat(mattermost): add native slash command support (refresh)#32467
feat(mattermost): add native slash command support (refresh)#32467mukhtharcm merged 31 commits intoopenclaw:mainfrom
Conversation
|
🤖 We're reviewing this PR with Aisle We're running a security check on the changes in this PR now. This usually takes a few minutes. ⌛ Progress:
Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T03:18:35Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T05:00:55Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T05:08:25Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T05:41:18Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T06:27:38Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T06:32:52Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-03T07:07:55Z |
openclaw-barnacle
bot
added
docs
Greptile SummaryThis PR adds opt-in native Mattermost slash command support: on connection, the bot registers Key changes:
Confidence Score: 4/5
Last reviewed commit: e7e5782 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/gateway/server-http.ts
Outdated
| // Non-channel plugin routes remain plugin-owned and must enforce | ||
| // their own auth when exposing sensitive functionality. | ||
| if (requestPath.startsWith("/api/channels/")) { | ||
| const mattermostCallbackPaths = new Set<string>(); | ||
| const defaultMmCallbackPath = "/api/channels/mattermost/command"; | ||
|
|
||
| const isMattermostCommandCallbackPath = (path: string): boolean => | ||
| path === defaultMmCallbackPath || path.startsWith("/api/channels/mattermost/"); | ||
|
|
||
| const normalizeCallbackPath = (value: unknown): string => { | ||
| const trimmed = typeof value === "string" ? value.trim() : ""; | ||
| if (!trimmed) { | ||
| return defaultMmCallbackPath; | ||
| } | ||
| return trimmed.startsWith("/") ? trimmed : `/${trimmed}`; | ||
| }; | ||
|
|
||
| const addMattermostCallbackPathIfValid = (path: string) => { | ||
| if (isMattermostCommandCallbackPath(path)) { | ||
| mattermostCallbackPaths.add(path); | ||
| } | ||
| }; | ||
|
|
||
| const tryAddCallbackUrlPath = (rawUrl: unknown) => { | ||
| if (typeof rawUrl !== "string") { | ||
| return; | ||
| } | ||
| const trimmed = rawUrl.trim(); | ||
| if (!trimmed) { | ||
| return; | ||
| } | ||
| try { | ||
| const pathname = new URL(trimmed).pathname; | ||
| if (pathname) { | ||
| addMattermostCallbackPathIfValid(pathname); | ||
| } | ||
| } catch { | ||
| // ignore | ||
| } | ||
| }; | ||
|
|
||
| const mmRaw = configSnapshot.channels?.mattermost as Record<string, unknown> | undefined; | ||
| const addMmCommands = (raw: unknown) => { | ||
| if (raw == null || typeof raw !== "object") { | ||
| return; | ||
| } | ||
| const commands = raw as Record<string, unknown>; | ||
| addMattermostCallbackPathIfValid(normalizeCallbackPath(commands?.callbackPath)); | ||
| tryAddCallbackUrlPath(commands?.callbackUrl); | ||
| }; | ||
|
|
||
| addMmCommands(mmRaw?.commands); | ||
| const accountsRaw = (mmRaw?.accounts ?? {}) as Record<string, unknown>; | ||
| for (const accountId of Object.keys(accountsRaw)) { | ||
| const accountCfg = accountsRaw[accountId] as Record<string, unknown> | undefined; | ||
| addMmCommands(accountCfg?.commands); | ||
| } | ||
|
|
||
| const isMattermostSlashCommandCallback = mattermostCallbackPaths.has(requestPath); | ||
| if (requestPath.startsWith("/api/channels/") && !isMattermostSlashCommandCallback) { | ||
| const token = getBearerToken(req); | ||
| const authResult = await authorizeGatewayConnect({ | ||
| auth: resolvedAuth, |
There was a problem hiding this comment.
Per-request Set construction on every channel request
The Mattermost callback path Set, all associated helper closures (isMattermostCommandCallbackPath, normalizeCallbackPath, addMattermostCallbackPathIfValid, tryAddCallbackUrlPath, addMmCommands), and the URL parsing for callbackUrl entries are all re-created on every incoming HTTP request that hits the /api/channels/ branch. Since configSnapshot rarely changes between requests, this work is redundant.
Consider computing the bypass-path set once when the handler is initialised (or on config change) and reusing it across requests:
// Outside the per-request handler, computed once from configSnapshot:
const mattermostCallbackPaths = buildMattermostCallbackPaths(configSnapshot);
// Then inside the handler, a simple O(1) lookup:
const isMattermostSlashCommandCallback = mattermostCallbackPaths.has(requestPath);For most deployments the overhead per request is just a few µs, but it's unnecessary given the config is stable between requests and the pattern is inconsistent with how other auth-bypass decisions are made in this file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-http.ts
Line: 503-564
Comment:
**Per-request Set construction on every channel request**
The Mattermost callback path `Set`, all associated helper closures (`isMattermostCommandCallbackPath`, `normalizeCallbackPath`, `addMattermostCallbackPathIfValid`, `tryAddCallbackUrlPath`, `addMmCommands`), and the URL parsing for `callbackUrl` entries are all re-created on every incoming HTTP request that hits the `/api/channels/` branch. Since `configSnapshot` rarely changes between requests, this work is redundant.
Consider computing the bypass-path set once when the handler is initialised (or on config change) and reusing it across requests:
```ts
// Outside the per-request handler, computed once from configSnapshot:
const mattermostCallbackPaths = buildMattermostCallbackPaths(configSnapshot);
// Then inside the handler, a simple O(1) lookup:
const isMattermostSlashCommandCallback = mattermostCallbackPaths.has(requestPath);
```
For most deployments the overhead per request is just a few µs, but it's unnecessary given the config is stable between requests and the pattern is inconsistent with how other auth-bypass decisions are made in this file.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
In the single-account fast-path the request is forwarded directly to the handler, which correctly short-circuits on non-POST methods before reading the body. In the multi-account branch, the full body is unconditionally buffered first — then the handler (which is called after token routing) will check For non-POST requests (e.g. health probes or Mattermost's GET reachability check) this means up to 64 KB is buffered unnecessarily. Adding an early method guard before the body-buffering loop would align the behaviour with the single-account path: // Multi-account: buffer the body, find the matching account by token,
// then replay the request to the correct handler.
if (req.method !== "POST") {
res.statusCode = 405;
res.setHeader("Allow", "POST");
res.end("Method Not Allowed");
return;
}
const chunks: Buffer[] = [];Prompt To Fix With AIThis is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/slash-state.ts
Line: 2270-2286
Comment:
**Body buffered before method check in multi-account path**
In the single-account fast-path the request is forwarded directly to the handler, which correctly short-circuits on non-POST methods before reading the body. In the multi-account branch, the full body is unconditionally buffered first — then the handler (which is called after token routing) will check `req.method` and return 405.
For non-POST requests (e.g. health probes or Mattermost's GET reachability check) this means up to 64 KB is buffered unnecessarily. Adding an early method guard before the body-buffering loop would align the behaviour with the single-account path:
```ts
// Multi-account: buffer the body, find the matching account by token,
// then replay the request to the correct handler.
if (req.method !== "POST") {
res.statusCode = 405;
res.setHeader("Allow", "POST");
res.end("Method Not Allowed");
return;
}
const chunks: Buffer[] = [];
```
How can I resolve this? If you propose a fix, please make it concise. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7e57820e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| throw err; | ||
| } | ||
|
|
||
| const existingByTrigger = new Map(existing.map((cmd) => [cmd.trigger, cmd])); |
There was a problem hiding this comment.
registerSlashCommands indexes existing team commands by trigger alone and then updates/deletes the matched entry without checking ownership. In teams that already have another integration using the same trigger (for example /oc_status), startup can repoint or remove that foreign command, breaking the other integration and permanently changing its callback. Reconciliation should only mutate commands that OpenClaw created (for example by validating creator_id or another ownership marker) and treat third-party trigger collisions as conflicts.
Useful? React with 👍 / 👎.
| commands: allRegistered, | ||
| log: (msg) => runtime.log?.(msg), | ||
| }); | ||
| throw err; |
There was a problem hiding this comment.
The catch block rethrows team-level registration errors, which makes setup all-or-nothing across all teams: if one team fails (such as missing permissions), previously successful registrations are cleaned up and slash commands are never activated anywhere. In multi-team deployments this causes a full outage even when some teams are valid; failures should be isolated per team so healthy teams remain usable.
Useful? React with 👍 / 👎.
mukhtharcm
force-pushed
the
refresh/mattermost-slash-16515
branch
2 times, most recently
from
5c4e5d5 to
785ed9d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 785ed9d047
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return allowFrom.some( | ||
| (entry) => entry === normalizedId || (normalizedName && entry === normalizedName), | ||
| ); |
There was a problem hiding this comment.
isSenderAllowed always treats senderName as an auth candidate, so slash callbacks can be authorized by mutable usernames even when channels.mattermost.dangerouslyAllowNameMatching is left at its safer default (false). In practice, if an operator uses username-style allowlist entries for DMs/groups, /oc_* slash commands will be accepted on name match paths that the regular Mattermost monitor intentionally gates behind the dangerous-name flag, which weakens the expected ID-based access control.
Useful? React with 👍 / 👎.
| const effectiveAllowFrom = Array.from(new Set([...configAllowFrom, ...storeAllowFrom])); | ||
| const effectiveGroupAllowFrom = Array.from( | ||
| new Set([ | ||
| ...(configGroupAllowFrom.length > 0 ? configGroupAllowFrom : configAllowFrom), | ||
| ...storeAllowFrom, |
There was a problem hiding this comment.
This merges storeAllowFrom (DM pairing approvals) into effectiveGroupAllowFrom, and that list is then used by the slash-command authorization gate, so a user paired in DM can become authorized to run group /oc_* commands without being in configured groupAllowFrom. The shared access-control helper explicitly documents that group command authorization must not inherit DM pairing-store approvals, so this introduces a privilege expansion for native slash commands in group/channel contexts.
Useful? React with 👍 / 👎.
mukhtharcm
force-pushed
the
refresh/mattermost-slash-16515
branch
from
785ed9d to
0522e85
Compare
mukhtharcm
force-pushed
the
refresh/mattermost-slash-16515
branch
from
0522e85 to
f028b77
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f028b7762c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Summary
Supersedes #18657 due to missing push permission on the original PR head repository.
This branch contains the prepared follow-up fixes:
Validation
Refs #16515