Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ type MCPClientManager interface {
IsPluginRegistered(pluginID string) bool

DiscoverPluginServerTools(ctx context.Context, userID string, cfg mcp.PluginServerConfig) ([]mcp.ToolInfo, error)

ReadUserAppResource(ctx context.Context, userID, serverOrigin, uri string) (*mcp.AppResource, error)
}

// ConfigStore provides read/write access to the plugin configuration in the database.
Expand Down Expand Up @@ -307,6 +309,7 @@ func (a *API) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Reques
router.GET("/mcp/user-preferences", a.handleGetUserPreferences)
router.PUT("/mcp/user-preferences", a.handlePutUserPreferences)
router.DELETE("/mcp/oauth/:serverName", a.handleDeleteUserMCPOAuth)
router.GET("/mcp/app-resource", a.handleGetMCPAppResource)

// Agent routes — authenticated. Free-tier instances (no multi-LLM license)
// can CRUD up to one self-service agent; the quota is enforced inside
Expand Down
49 changes: 49 additions & 0 deletions api/api_conversation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/gin-gonic/gin"
"github.com/mattermost/mattermost-plugin-agents/v2/conversation"
"github.com/mattermost/mattermost-plugin-agents/v2/llm"
"github.com/mattermost/mattermost-plugin-agents/v2/store"
"github.com/mattermost/mattermost/server/public/model"
"github.com/mattermost/mattermost/server/public/plugin"
Expand Down Expand Up @@ -416,3 +417,51 @@ func mustMarshalBlocks(t *testing.T, blocks []conversation.ContentBlock) json.Ra
require.NoError(t, err)
return data
}

func TestHandleGetConversationFiltersUIMetaForNonOwner(t *testing.T) {
gin.SetMode(gin.ReleaseMode)
gin.DefaultWriter = io.Discard

channelID := testChannelID
uiMeta := &llm.ToolUIMeta{ResourceURI: "ui://srv/app.html"}
blocks := mustMarshalBlocks(t, []conversation.ContentBlock{
{
Type: conversation.BlockTypeToolUse, ID: "tc_priv", Name: "demo",
Input: json.RawMessage(`{}`), Shared: conversation.BoolPtr(false), UIMeta: uiMeta,
},
{
Type: conversation.BlockTypeToolUse, ID: "tc_shared", Name: "demo",
Input: json.RawMessage(`{}`), Shared: conversation.BoolPtr(true), UIMeta: uiMeta,
},
})

e := SetupTestEnvironment(t)
defer e.Cleanup(t)
e.conversationStore.conversations["conv-ui-meta"] = &store.Conversation{
ID: "conv-ui-meta",
UserID: testUserID,
BotID: testBotUserID,
ChannelID: &channelID,
}
e.conversationStore.turns["conv-ui-meta"] = []store.Turn{
{ID: "turn-1", ConversationID: "conv-ui-meta", Role: "assistant", Content: blocks, Sequence: 1},
}
e.mockAPI.On("HasPermissionToChannel", testOtherUserID, channelID, model.PermissionReadChannel).Return(true)
e.mockAPI.On("LogError", mock.Anything).Maybe()

request := httptest.NewRequest(http.MethodGet, "/conversations/conv-ui-meta", nil)
request.Header.Add("Mattermost-User-ID", testOtherUserID)
recorder := httptest.NewRecorder()
e.api.ServeHTTP(&plugin.Context{}, recorder, request)
require.Equal(t, http.StatusOK, recorder.Result().StatusCode)

var response ConversationResponse
require.NoError(t, json.NewDecoder(recorder.Result().Body).Decode(&response))
require.Len(t, response.Turns, 1)

var got []conversation.ContentBlock
require.NoError(t, json.Unmarshal(response.Turns[0].Content, &got))
require.Len(t, got, 2)
assert.Nil(t, got[0].UIMeta, "unshared tool_use must strip ui_meta for non-owner")
assert.Equal(t, uiMeta, got[1].UIMeta, "shared tool_use must keep ui_meta for non-owner")
}
176 changes: 176 additions & 0 deletions api/api_mcp_app_resource.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

package api

import (
"errors"
"fmt"
"net/http"

"github.com/gin-gonic/gin"
"github.com/mattermost/mattermost-plugin-agents/v2/conversation"
"github.com/mattermost/mattermost-plugin-agents/v2/mcp"
"github.com/mattermost/mattermost/server/public/model"
)

const (
appResourceErrInvalidRequest = "invalid_request"
appResourceErrNotFound = "not_found"
appResourceErrForbidden = "forbidden"
appResourceErrAuthRequired = "mcp_auth_required"
appResourceErrUpstreamUnreachable = "upstream_unreachable"
appResourceErrInvalidResourceMime = "invalid_resource_mime"
)

// AppResourceContents is one contents[] entry in the MCP ReadResourceResult
// wire shape returned by GET /mcp/app-resource on success.
type AppResourceContents struct {
URI string `json:"uri"`
MIMEType string `json:"mimeType"`
Text string `json:"text"`
Meta *AppResourceMeta `json:"_meta,omitempty"`
}

// AppResourceMeta is the MCP `_meta` object on a resource contents item.
type AppResourceMeta struct {
UI *mcp.AppResourceUIMeta `json:"ui,omitempty"`
}

// AppResourceResponse is the success JSON shape for GET /mcp/app-resource.
// It mirrors MCP ReadResourceResult so Phase 1c's onReadResource callback can
// return response.json() directly to @mcp-ui/client.
type AppResourceResponse struct {
Contents []AppResourceContents `json:"contents"`
}

// AppResourceErrorResponse is the JSON error shape for GET /mcp/app-resource.
// ErrorCode drives webapp behavior: mcp_auth_required renders "Connect to
// view" with AuthURL; forbidden renders the no-access popover.
//
// D5 state 3 ("can never gain access") is approximated client-side: after a
// shared-result onlooker receives repeated mcp_auth_required responses and
// OAuth fails/denies, the webapp renders the state-3 popover. The server has
// no distinct definitive-rejection code beyond that flow.
type AppResourceErrorResponse struct {
ErrorCode string `json:"error_code"`
Message string `json:"message"`
AuthURL string `json:"auth_url,omitempty"`
}

func writeAppResourceError(c *gin.Context, status int, code, message, authURL string) {
c.JSON(status, AppResourceErrorResponse{
ErrorCode: code,
Message: message,
AuthURL: authURL,
})
}

func (a *API) handleGetMCPAppResource(c *gin.Context) {
// Explicit non-checks: no license gate (viewing an already-executed tool's
// UI is a read, consistent with un-gated GET /conversations/:id; execution
// was already license-gated at handleToolCall), no EnableChannelMentionToolCalling
// gate (same reason), no tool-status gate (D3's Success/AutoApproved gating
// is a render-time webapp concern; the resource template itself is not
// result data).
//
// D6(b) uses the tool_use block's Shared flag — the same flag
// FilterForNonRequester uses when exposing ui_meta — so the invariant is
// exact: if GET /conversations/:id shows ui_meta to the caller, this
// fetch will not 403 for the share check.
userID := c.GetHeader("Mattermost-User-Id")
postID := c.Query("post_id")
toolCallID := c.Query("tool_call_id")
if postID == "" || toolCallID == "" || !model.IsValidId(postID) {
writeAppResourceError(c, http.StatusBadRequest, appResourceErrInvalidRequest, "post_id and tool_call_id are required", "")
return
}

post, err := a.pluginAPI.Post.GetPost(postID)
if err != nil {
writeAppResourceError(c, http.StatusNotFound, appResourceErrNotFound, "post not found", "")
return
}

if !a.pluginAPI.User.HasPermissionToChannel(userID, post.ChannelId, model.PermissionReadChannel) {
writeAppResourceError(c, http.StatusForbidden, appResourceErrForbidden, "permission denied", "")
return
}

turn, err := a.conversationStore.GetTurnByPostID(postID)
if err != nil || turn == nil {
writeAppResourceError(c, http.StatusNotFound, appResourceErrNotFound, "turn not found", "")
return
}

conv, err := a.conversationStore.GetConversation(turn.ConversationID)
if err != nil {
c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to get conversation: %w", err))
return
}
requesterID := conv.UserID

turns, err := a.conversationStore.GetTurnsForConversation(conv.ID)
if err != nil {
c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to get turns: %w", err))
return
}

toolUse, _, findErr := conversation.FindToolCallBlocks(turns, postID, toolCallID)
if findErr != nil {
if errors.Is(findErr, conversation.ErrAmbiguousToolCallID) {
writeAppResourceError(c, http.StatusNotFound, appResourceErrNotFound, "ambiguous tool call", "")
return
}
writeAppResourceError(c, http.StatusNotFound, appResourceErrNotFound, "tool call not found", "")
return
}
if toolUse.UIMeta == nil || toolUse.UIMeta.ResourceURI == "" {
writeAppResourceError(c, http.StatusNotFound, appResourceErrNotFound, "tool call has no app resource", "")
return
}

if userID != requesterID {
if toolUse.Shared == nil || !*toolUse.Shared {
writeAppResourceError(c, http.StatusForbidden, appResourceErrForbidden, "tool result is not shared", "")
return
}
}

res, err := a.mcpClientManager.ReadUserAppResource(c.Request.Context(), userID, toolUse.ServerOrigin, toolUse.UIMeta.ResourceURI)
if err != nil {
var oauthErr *mcp.OAuthNeededError
if errors.As(err, &oauthErr) {
writeAppResourceError(c, http.StatusUnauthorized, appResourceErrAuthRequired, "MCP authentication required", oauthErr.AuthURL())
return
}
if errors.Is(err, mcp.ErrServerNotConfigured) {
writeAppResourceError(c, http.StatusNotFound, appResourceErrNotFound, "server no longer configured", "")
return
}
var invalidErr *mcp.InvalidAppResourceError
if errors.As(err, &invalidErr) {
a.pluginAPI.Log.Error("Invalid MCP app resource",
"userID", userID, "serverOrigin", toolUse.ServerOrigin, "uri", toolUse.UIMeta.ResourceURI, "error", invalidErr)
writeAppResourceError(c, http.StatusBadGateway, appResourceErrInvalidResourceMime, "invalid app resource", "")
return
}
a.pluginAPI.Log.Error("MCP app resource upstream failure",
"userID", userID, "serverOrigin", toolUse.ServerOrigin, "uri", toolUse.UIMeta.ResourceURI, "error", err)
writeAppResourceError(c, http.StatusBadGateway, appResourceErrUpstreamUnreachable, "MCP server unreachable", "")
return
}
Comment on lines +140 to +162

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -A5 -B5 'context.WithTimeout|context.WithDeadline' mcp/resources.go mcp/client.go

Repository: mattermost/mattermost-plugin-agents

Length of output: 173


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== file list ==\n'
git ls-files | rg '(^|/)(mcp|api)/.*\.(go|ts|tsx)$|^mcp/|^api/api_mcp_app_resource\.go$'

printf '\n== locate ReadUserAppResource ==\n'
rg -n 'ReadUserAppResource|WithTimeout|WithDeadline|context\.Background|context\.TODO' api mcp

printf '\n== map candidate files ==\n'
for f in api/api_mcp_app_resource.go mcp/resources.go mcp/client.go; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    ast-grep outline "$f" --view compact || true
  fi
done

printf '\n== relevant slices ==\n'
for f in mcp/resources.go mcp/client.go api/api_mcp_app_resource.go; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    nl -ba "$f" | sed -n '1,260p'
  fi
done

Repository: mattermost/mattermost-plugin-agents

Length of output: 18718


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== ReadUserAppResource implementation =='
sed -n '230,320p' mcp/resources.go

echo
echo '== client manager timeout-related path =='
sed -n '130,190p' mcp/user_clients.go

echo
echo '== client manager wrapper =='
sed -n '1,120p' mcp/client_manager.go

echo
echo '== client read path =='
sed -n '1,260p' mcp/client.go | sed -n '1,260p'

Repository: mattermost/mattermost-plugin-agents

Length of output: 17777


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== user_clients remote connect =='
sed -n '1,140p' mcp/user_clients.go
echo
sed -n '140,240p' mcp/user_clients.go

echo
echo '== client session setup =='
sed -n '260,520p' mcp/client.go

echo
echo '== search for connect/read timeouts in mcp package =='
rg -n 'WithTimeout|WithDeadline|Deadline|timeout' mcp/client.go mcp/user_clients.go mcp/http_client.go mcp/plugin_roundtripper.go mcp/oauth_transport.go mcp/dcrp.go

Repository: mattermost/mattermost-plugin-agents

Length of output: 15630


Add a bounded timeout to the remote MCP app-resource path

ReadUserAppResource passes cacheableContext(ctx) into ConnectToRemoteServer(...), which drops request cancellation. If the MCP server hangs during connect or resources/read, this request can block indefinitely; use a per-call timeout here instead of relying on the client connection.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/api_mcp_app_resource.go` around lines 140 - 162, Add a bounded per-call
timeout around the ReadUserAppResource invocation, deriving the context from
c.Request.Context() and passing the timed context instead of the uncancelled
request context. Ensure the timeout covers both remote connection and
resources/read operations while preserving the existing error handling and
response behavior.


var meta *AppResourceMeta
if res.UIMeta != nil {
meta = &AppResourceMeta{UI: res.UIMeta}
}
c.JSON(http.StatusOK, AppResourceResponse{
Contents: []AppResourceContents{{
URI: res.URI,
MIMEType: res.MIMEType,
Text: res.HTML,
Meta: meta,
}},
})
}
Loading
Loading