fix: harden Mattermost slash callback auth#65655
fix: harden Mattermost slash callback auth#65655coygeek wants to merge 1 commit intoopenclaw:mainfrom
Conversation
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR hardens Mattermost native slash command auth in two ways: it fails closed on registration when Confidence Score: 5/5Safe to merge; both security fixes are correct and well-tested, remaining findings are P2 style concerns only. All active findings are P2 (silent optional bypass is a latent risk not a current defect; explicit-HTTP gap is a hardening suggestion). Production code always provides No files require special attention beyond the two P2 comments on Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/slash-http.ts
Line: 55
Comment:
**Silent bypass when `commandTokenTriggers` is absent**
`commandTokenTriggers` is typed as optional, so when it's `undefined`, `resolveRegisteredCommandTrigger` returns `undefined` and the `if (expectedTrigger && expectedTrigger !== trigger)` guard is never entered — cross-trigger replay protection is silently dropped. Production code via `activateSlashCommands` always provides the map, but a handler constructed without it (e.g., in a future code path) would revert to token-only validation without any warning or compile-time signal. Making the field required, or at minimum adding a fallthrough `runtime.error` when the map is absent, would prevent silent regressions.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor-slash.ts
Line: 171-176
Comment:
**No warning for explicitly-configured non-HTTPS callback URLs**
The new guard fires only when `callbackUrl` is *omitted* (`!slashConfig.callbackUrl`). If an operator explicitly sets `callbackUrl: "http://public-server.example.com/slash"`, both conditions are false, the block is skipped, and registration proceeds silently over HTTP. `warnOnSuspiciousCallbackUrl` won't catch this either — it only warns about loopback hosts. Adding a warning (or a fail-closed error matching the derived-URL path) when an explicit `callbackUrl` resolves to non-HTTPS would close this gap and keep the behaviour consistent with the documented intent.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: harden Mattermost slash callback au..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Expected token from registered commands (for validation). */ | ||
| commandTokens: Set<string>; | ||
| /** Exact trigger expected for each registered token. */ | ||
| commandTokenTriggers?: ReadonlyMap<string, string>; |
There was a problem hiding this comment.
commandTokenTriggers is absent
commandTokenTriggers is typed as optional, so when it's undefined, resolveRegisteredCommandTrigger returns undefined and the if (expectedTrigger && expectedTrigger !== trigger) guard is never entered — cross-trigger replay protection is silently dropped. Production code via activateSlashCommands always provides the map, but a handler constructed without it (e.g., in a future code path) would revert to token-only validation without any warning or compile-time signal. Making the field required, or at minimum adding a fallthrough runtime.error when the map is absent, would prevent silent regressions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/slash-http.ts
Line: 55
Comment:
**Silent bypass when `commandTokenTriggers` is absent**
`commandTokenTriggers` is typed as optional, so when it's `undefined`, `resolveRegisteredCommandTrigger` returns `undefined` and the `if (expectedTrigger && expectedTrigger !== trigger)` guard is never entered — cross-trigger replay protection is silently dropped. Production code via `activateSlashCommands` always provides the map, but a handler constructed without it (e.g., in a future code path) would revert to token-only validation without any warning or compile-time signal. Making the field required, or at minimum adding a fallthrough `runtime.error` when the map is absent, would prevent silent regressions.
How can I resolve this? If you propose a fix, please make it concise.| if (!slashConfig.callbackUrl && !isHttpsUrl(slashCallbackUrl)) { | ||
| params.runtime.error?.( | ||
| `mattermost: native slash commands require an explicit HTTPS channels.mattermost.commands.callbackUrl; refusing derived callback ${slashCallbackUrl}`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The new guard fires only when callbackUrl is omitted (!slashConfig.callbackUrl). If an operator explicitly sets callbackUrl: "http://public-server.example.com/slash", both conditions are false, the block is skipped, and registration proceeds silently over HTTP. warnOnSuspiciousCallbackUrl won't catch this either — it only warns about loopback hosts. Adding a warning (or a fail-closed error matching the derived-URL path) when an explicit callbackUrl resolves to non-HTTPS would close this gap and keep the behaviour consistent with the documented intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/mattermost/src/mattermost/monitor-slash.ts
Line: 171-176
Comment:
**No warning for explicitly-configured non-HTTPS callback URLs**
The new guard fires only when `callbackUrl` is *omitted* (`!slashConfig.callbackUrl`). If an operator explicitly sets `callbackUrl: "http://public-server.example.com/slash"`, both conditions are false, the block is skipped, and registration proceeds silently over HTTP. `warnOnSuspiciousCallbackUrl` won't catch this either — it only warns about loopback hosts. Adding a warning (or a fail-closed error matching the derived-URL path) when an explicit `callbackUrl` resolves to non-HTTPS would close this gap and keep the behaviour consistent with the documented intent.
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: found issues before merge. What this changes: This PR updates Mattermost slash command docs, registration, HTTP handler, state, and tests to fail closed for non-HTTPS derived callbacks and bind registered command tokens to slash triggers. Maintainer follow-up before merge: This is security-sensitive Mattermost auth work, paired with open bug #65624 and stale against merged #72923, so maintainers should choose whether to rebase this branch, close it in favor of a narrow follow-up, or make a policy decision for non-HTTPS callback URLs. Security review: Security review needs attention: The diff improves Mattermost slash callback auth, but leaves concrete cleartext and silent-bypass gaps in the proposed hardening. Review findings:
Review detailsBest possible solution: Keep the current #72923 command-validation architecture and add a narrow follow-up on top of main that enforces the chosen Mattermost callback URL transport policy, with docs and focused tests for omitted Do we have a high-confidence way to reproduce the issue? Yes. A source-level reproduction is high confidence: enable Mattermost native slash commands, omit Is this the best way to solve the issue? No, not as submitted. The PR addresses the right security surface, but current main has a stronger replacement for token replay validation, while the PR's callback URL guard is incomplete and should be rebased or replaced with a narrow policy fix on current main. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4babd925c40d. |
1 participant
Fix Summary
Mattermost native slash commands were insecure in two ways: when
commands.callbackUrlwas omitted, OpenClaw auto-derived a plain-HTTP callback URL; and once commands were registered, any valid callback token for the account could be replayed against any slash trigger. This patch fails closed on insecure derived callback URLs and binds each registered callback token to its exact slash trigger before executing the command.Issue Linkage
Fixes #65624
Security Snapshot
Implementation Details
Files Changed
docs/channels/mattermost.md(+4/-1)docs/gateway/configuration-reference.md(+2/-0)extensions/mattermost/src/mattermost/monitor-slash.test.ts(+25/-11)extensions/mattermost/src/mattermost/monitor-slash.ts(+13/-0)extensions/mattermost/src/mattermost/slash-http.send-config.test.ts(+2/-0)extensions/mattermost/src/mattermost/slash-http.test.ts(+17/-2)extensions/mattermost/src/mattermost/slash-http.ts(+24/-1)extensions/mattermost/src/mattermost/slash-state.ts(+12/-0)Technical Analysis
The slash registration path previously accepted a derived
http://...callback when operators omittedcommands.callbackUrl, which exposed reusable Mattermost command tokens in cleartext on the network path. Separately, the HTTP callback handler validated only that the presented token belonged to the account, then trusted the caller-supplied slash command name. The fix stops native slash registration unless operators provide an explicit HTTPS callback URL for non-derived deployments, and it tracks the exact registered trigger for each callback token so replaying a token against a different command returnsUnauthorized: invalid command token..Validation Evidence
pnpm test extensions/mattermost/src/mattermost/slash-http.test.tspnpm test extensions/mattermost/src/mattermost/slash-http.send-config.test.tspnpm test extensions/mattermost/src/mattermost/monitor-slash.test.tsRisk and Compatibility
commands.callbackUrl; valid callbacks keep working once configuredAI-Assisted Disclosure