Conversation
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
…iscovery Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
There was a problem hiding this comment.
Stale comment
Security Review — No Actionable Findings
I reviewed the full diff focusing on the areas outlined in the review checklist. Here is a summary of the analysis:
Bridge tool execution (new
allowed_toolsfeature):
- The bridge remains behind
interPluginAuthorizationRequiredmiddleware; only trusted plugins can invoke it.allowed_toolsis correctly rejected on service endpoints and only accepted on agent endpoints.- Tool scoping is enforced server-side: requested tools must exist in the agent's eligible tool set, tools-disabled agents reject the request, and empty/blank tool names are rejected.
WithAutoRunToolsis applied only when the calling plugin explicitly provides anallowed_toolslist, preserving the default tools-disabled behavior for bridge requests without it.- User/channel ID validation (
ValidateID) is applied consistently via middleware and request-body checks across all endpoints.Automation tools (new
create_automation,update_automation,delete_automation,list_automations):
- All CRUD operations forward the user's own Bearer token to the automation plugin API via
doAutomationRequest, so the automation plugin enforces per-user and per-channel authorization.shouldShowAutomationToolsrestricts visibility in public/private channels to channel admins and sysadmins, defaulting to visible in DMs/groups where the backend enforces per-channel checks.- The
isAutomationPluginInstalledprobe correctly uses an unauthenticated request (checking route existence only) and doesn't leak credentials.Agent discovery (
list_agents,handleGetAgentTools):
- Both are read-only discovery endpoints behind inter-plugin auth. User permission filtering is applied when
user_idis supplied.File access in bridge posts:
convertBridgePostsToInternaluses server-level file access, which is pre-existing behavior unchanged by this PR. The bridge contract places permission responsibility on the calling plugin.Reviewed and found adequate: ID validation, auth token scoping, tool store isolation, service vs. agent endpoint separation,
WithToolsDisabledplacement, automation tool filtering middleware, and prompt injection mitigations (user profile field sanitization was already in place).Sent by Cursor Automation: Find vulnerabilities
| // with an auth error), false if it 404s or is unreachable. | ||
| func (p *MattermostToolProvider) isAutomationPluginInstalled() bool { | ||
| reqURL := p.automationAPIURL("/flows") | ||
| req, err := http.NewRequest(http.MethodGet, reqURL, nil) |
There was a problem hiding this comment.
We were using some client4 doRequestWithHeaders function before but that changed in the latest release where it automatically prepends <serverURL>/api/v4 so it no longer works for us with plugins
|
|
||
| // Add middleware to dynamically filter automation tools from tools/list | ||
| // when the channel automation plugin is not installed. | ||
| mcpServer.AddReceivingMiddleware(p.automationToolFilterMiddleware()) |
There was a problem hiding this comment.
Moved into a middleware to check for plugin existence when an agent requests a tool list. Previously if the automation plugin was uploaded after the agents plugin started up, we would never find the automation tools. We are caching the 'plugin installed` flow for 30 mins so we don't make redundant requests
There was a problem hiding this comment.
Stale comment
Security Review — No Actionable Findings
Re-reviewed the updated diff covering the new
allowed_toolsbridge feature, automation CRUD tools, agent discovery, and related refactors. All prior conclusions remain valid; no new vulnerabilities introduced.Key controls verified:
- Bridge auth boundary intact: All new endpoints (
/agents/:agent/tools, agent completion withallowed_tools) remain behindinterPluginAuthorizationRequired. The calling plugin owns user permission verification per the bridge contract.- Tool scoping enforced server-side:
prepareAgentBridgeCompletionvalidates each requested tool exists in the agent's eligible set, rejects tools-disabled agents, and rejects empty/blank tool names.WithToolsDisabled()is correctly applied when noallowed_toolsis provided, and explicitly on both service endpoints.WithToolsDisabled()removal fromconvertRequestToLLMOptionsis safe: Coverage is maintained — agent path handles it inprepareAgentBridgeCompletion(line 302-303), and service streaming/non-streaming handlers append it explicitly (lines 728, 808).- Automation tools delegate auth correctly: All CRUD operations in
toolCreateAutomation,toolUpdateAutomation,toolDeleteAutomation, andtoolListAutomationsforward the user's own Bearer token viadoAutomationRequest. The automation plugin enforces per-user and per-channel authorization.shouldShowAutomationToolsgates tool visibility to channel admins/sysadmins in public/private channels.isAutomationPluginInstalledprobe: Uses an unauthenticated request to check route existence only.mmServerURLis server-configured, not user-controlled. No SSRF vector.list_agentstool: Uses the user's auth token to call/ai_bots, which is behindMattermostAuthorizationRequired. Properly scoped.- Input validation consistent:
ValidateIDapplied via middleware (validateAgentParam,validateUserIDQuery) and in request body parsing (validateCompletionRequestIDs).url.QueryEscapeused for channel_id query parameter in automation list.toolCreatePostAsUserURL change:mmInternalServerURLwas consolidated intommServerURLwhich still resolves to the internal URL when configured. No behavioral change.Sent by Cursor Automation: Find vulnerabilities
Resolve merge conflicts in api/api_test.go and add missing fmt import in llm/stream.go.
There was a problem hiding this comment.
Security Review — No Actionable Findings
Re-reviewed the updated diff covering the allowed_tools bridge feature, automation CRUD tools, agent/tool discovery endpoints, and associated refactors. No medium, high, or critical vulnerabilities identified.
Controls verified:
- Inter-plugin auth boundary: All new bridge endpoints (
/agents/:agent/tools, agent completion withallowed_tools) remain behindinterPluginAuthorizationRequired. Tool scoping and execution trust follows the established bridge contract where the calling plugin owns user permission verification. - Server-side tool scoping:
prepareAgentBridgeCompletionvalidates every requested tool against the agent's eligible set, rejects agents with tools disabled, rejects empty/blank tool names vianormalizeAllowedTools, and appliesWithToolsDisabled()when noallowed_toolsis provided. Service endpoints explicitly rejectallowed_toolsand appendWithToolsDisabled(). WithToolsDisabled()removal fromconvertRequestToLLMOptionsis safe: The agent path handles it inprepareAgentBridgeCompletion(lines 302-303), and both service handlers append it explicitly (lines 728, 808).- Automation tool auth delegation: All CRUD operations (
toolCreateAutomation,toolUpdateAutomation,toolDeleteAutomation,toolListAutomations) forward the user's own Bearer token viadoAutomationRequest. The automation plugin enforces per-user and per-channel authorization server-side. - Automation tool visibility gating:
shouldShowAutomationToolsrestricts tool visibility to sysadmins and channel admins in public/private channels. Thechannel == nilcase (DMs, bridge requests) returnstrueby design — the automation plugin backend enforces per-channel permissions at execution time. isAutomationPluginInstalledprobe: Uses server-configuredmmServerURL(not user-controlled) with an unauthenticated request to check route existence only. No SSRF vector.list_agentstool: Uses the user's auth token to call/ai_bots, which is behindMattermostAuthorizationRequired. Properly scoped.- Input validation:
ValidateIDapplied consistently via middleware (validateAgentParam,validateUserIDQuery) and request-body checks (validateCompletionRequestIDs).url.QueryEscapeused for channel_id query parameter. mmServerURLconsolidation:mmInternalServerURLmerged intommServerURLwhich still resolves to the internal URL when configured. ThetoolCreatePostAsUserURL change is functionally equivalent.- No secrets exposure: No hardcoded credentials, tokens, or keys in the diff. Test tokens are isolated to test helpers.
Sent by Cursor Automation: Find vulnerabilities
| mcpTools, mcpErrors := b.mcpToolProvider.GetToolsForUser(userID) | ||
|
|
||
| // Filter out automation tools for users without channel admin (or sysadmin) permission | ||
| showAutomation := shouldShowAutomationTools(b.pluginAPI, userID, c.Channel) |
There was a problem hiding this comment.
I think we still want to filter out tools that you don't even have permission to use, even from search. Let's go ahead and pass whatever we need to the MCP server to make this determination. This also means that we can make this determination for external MCP users as well as this solution currently does not cover those.
|
@crspeller I also had to fix an issue where we need to include a server name when choosing a tools to include in |
|
@BenCookie95 I think some merge conflicts are causing CI to fail. Need to fix those before merge. |
…ls to fetch instructions from the automation plugin
|
@crspeller Just re-requesting review due to a few minor changes here: 880cf89. Like we discussed on Tuesday, the bridge derives the server origin itself so the caller doesn't need to know it. Also, moved automation creation instructions over to the automation plugin. This still needs to finish security review as well |


Summary
allowed_tools. This allows completion calls to use from tools from a pre-defined list. This list is checked at runtime each time to ensure the requesting user actually permission to use those tools. You can service account tools if you don't pass a requesting userIDTicket Link
https://mattermost.atlassian.net/browse/MM-67712
Screenshots
Release Note
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation