fix(coderd): enforce Force On MCP server policy on the backend (#27990)

2026-09-24 15:04:27 +08:00 · 2026-08-11 12:02:21 +02:00
parent 496ed757ed
commit 91d3027498
4 changed files with 465 additions and 19 deletions
@@ -932,6 +932,66 @@ func TestPostChats(t *testing.T) {
 	})
 }

+// TestChats_ForceOnMCPServerEnforced is the endpoint-level regression
+// test for Cure53 CDM-02-010: a regular user who strips force_on MCP
+// server IDs from mcp_server_ids when creating a chat or sending a
+// message must not be able to exclude those servers.
+func TestChats_ForceOnMCPServerEnforced(t *testing.T) {
+	t.Parallel()
+
+	ctx := testutil.Context(t, testutil.WaitLong)
+	client := newChatClient(t)
+	firstUser := coderdtest.CreateFirstUser(t, client.Client)
+	_ = createChatModelConfig(t, client)
+
+	// An admin marks an MCP server as Force On.
+	forced, err := client.Client.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{
+		DisplayName:   "Forced Server",
+		Slug:          "forced-server",
+		Transport:     "streamable_http",
+		URL:           "https://mcp.example.com/forced",
+		AuthType:      "none",
+		Availability:  "force_on",
+		Enabled:       true,
+		ToolAllowList: []string{},
+		ToolDenyList:  []string{},
+	})
+	require.NoError(t, err)
+
+	// A regular member tampers with the request by clearing
+	// mcp_server_ids (Cure53 CDM-02-010 reproduction).
+	memberClientRaw, _ := coderdtest.CreateAnotherUser(t, client.Client, firstUser.OrganizationID, rbac.ScopedRoleAgentsAccess(firstUser.OrganizationID))
+	memberClient := codersdk.NewExperimentalClient(memberClientRaw)
+
+	chat, err := memberClient.CreateChat(ctx, codersdk.CreateChatRequest{
+		OrganizationID: firstUser.OrganizationID,
+		Content: []codersdk.ChatInputPart{{
+			Type: codersdk.ChatInputPartTypeText,
+			Text: "test message",
+		}},
+		MCPServerIDs: []uuid.UUID{},
+	})
+	require.NoError(t, err)
+	require.Contains(t, chat.MCPServerIDs, forced.ID,
+		"force_on MCP server must be enforced on chat creation")
+
+	// Sending a message with an emptied list must not remove the
+	// forced server either.
+	_, err = memberClient.CreateChatMessage(ctx, chat.ID, codersdk.CreateChatMessageRequest{
+		Content: []codersdk.ChatInputPart{{
+			Type: codersdk.ChatInputPartTypeText,
+			Text: "second message",
+		}},
+		MCPServerIDs: &[]uuid.UUID{},
+	})
+	require.NoError(t, err)
+
+	chatResult, err := memberClient.GetChat(ctx, chat.ID)
+	require.NoError(t, err)
+	require.Contains(t, chatResult.MCPServerIDs, forced.ID,
+		"force_on MCP server must survive a tampered mcp_server_ids update")
+}
+
 func TestPostChats_ClientType(t *testing.T) {
 	t.Parallel()

@@ -1202,6 +1202,36 @@ type PromoteQueuedResult struct {
 	PromotedMessage database.ChatMessage
 }

+// enforceForcedMCPServerIDs appends the ID of every enabled Force On
+// MCP server config missing from ids. Force On availability is a
+// server-side policy: callers must not be able to exclude such
+// servers by stripping IDs from a request (Cure53 CDM-02-010). The
+// forced set is read with daemon scope because regular users cannot
+// read MCP server configs directly.
+func enforceForcedMCPServerIDs(ctx context.Context, store database.Store, ids []uuid.UUID) ([]uuid.UUID, error) {
+	//nolint:gocritic // Non-admin users need chatd-scoped config reads here.
+	forced, err := store.GetForcedMCPServerConfigs(dbauthz.AsChatd(ctx))
+	if err != nil {
+		// Fail closed: proceeding without the forced set would
+		// silently bypass a security policy.
+		return nil, xerrors.Errorf("get forced MCP server configs: %w", err)
+	}
+	merged := slices.Clone(ids)
+	if merged == nil {
+		merged = []uuid.UUID{}
+	}
+	seen := make(map[uuid.UUID]struct{}, len(merged))
+	for _, id := range merged {
+		seen[id] = struct{}{}
+	}
+	for _, cfg := range forced {
+		if _, ok := seen[cfg.ID]; !ok {
+			merged = append(merged, cfg.ID)
+		}
+	}
+	return merged, nil
+}
+
 // CreateChat creates a chat with its initial history through
 // chatstate.CreateChat. The new chat starts in `running` status per
 // the chat execution state model. Ownership hints wake chat workers.
@@ -1224,6 +1254,14 @@ func (p *Server) CreateChat(ctx context.Context, opts CreateOptions) (database.C
 	if opts.MCPServerIDs == nil {
 		opts.MCPServerIDs = []uuid.UUID{}
 	}
+	// Force On MCP servers are enforced server-side so a caller
+	// cannot exclude them by stripping IDs from the request
+	// (Cure53 CDM-02-010).
+	enforcedMCPServerIDs, err := enforceForcedMCPServerIDs(ctx, p.db, opts.MCPServerIDs)
+	if err != nil {
+		return database.Chat{}, err
+	}
+	opts.MCPServerIDs = enforcedMCPServerIDs
 	if opts.Labels == nil {
 		opts.Labels = database.StringMap{}
 	}
@@ -1473,9 +1511,16 @@ func (p *Server) SendMessage(
 					slog.F("chat_id", opts.ChatID),
 				)
 			} else {
+				// Force On MCP servers are enforced server-side so a
+				// caller cannot remove them by tampering with the
+				// update (Cure53 CDM-02-010).
+				enforcedIDs, enforceErr := enforceForcedMCPServerIDs(ctx, store, *requestedMCPServerIDs)
+				if enforceErr != nil {
+					return enforceErr
+				}
 				lockedChat, err = store.UpdateChatMCPServerIDs(ctx, database.UpdateChatMCPServerIDsParams{
 					ID:           opts.ChatID,
-					MCPServerIDs: *requestedMCPServerIDs,
+					MCPServerIDs: enforcedIDs,
 				})
 				if err != nil {
 					return xerrors.Errorf("update chat mcp server ids: %w", err)
@@ -0,0 +1,302 @@
+package chatd_test
+
+// Regression tests for Cure53 CDM-02-010: the Force On MCP server
+// availability policy must be enforced on the backend. A client that
+// omits force_on entries from mcp_server_ids when creating a chat or
+// sending a message must not be able to exclude those servers from
+// the conversation.
+
+import (
+	"context"
+	"net/http/httptest"
+	"sync"
+	"testing"
+
+	"github.com/google/uuid"
+	mcpgo "github.com/mark3labs/mcp-go/mcp"
+	mcpserver "github.com/mark3labs/mcp-go/server"
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbgen"
+	"github.com/coder/coder/v2/coderd/database/dbtestutil"
+	"github.com/coder/coder/v2/coderd/x/chatd"
+	"github.com/coder/coder/v2/coderd/x/chatd/chattest"
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/testutil"
+)
+
+// newEchoMCPTestServer starts an MCP test server exposing an "echo"
+// tool and returns its base URL.
+func newEchoMCPTestServer(t *testing.T, name string) string {
+	t.Helper()
+	srv := mcpserver.NewMCPServer(name, "1.0.0")
+	srv.AddTools(mcpserver.ServerTool{
+		Tool: mcpgo.NewTool("echo",
+			mcpgo.WithDescription("Echoes the input"),
+			mcpgo.WithString("input",
+				mcpgo.Description("The input string"),
+				mcpgo.Required(),
+			),
+		),
+		Handler: func(_ context.Context, req mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) {
+			input, _ := req.GetArguments()["input"].(string)
+			return mcpgo.NewToolResultText("echo: " + input), nil
+		},
+	})
+	ts := httptest.NewServer(mcpserver.NewStreamableHTTPServer(srv))
+	t.Cleanup(ts.Close)
+	return ts.URL
+}
+
+// newToolRecordingOpenAI returns a mock OpenAI URL that answers every
+// streamed call with plain text and records the tool names offered on
+// each streamed call, plus an accessor for the recorded calls.
+func newToolRecordingOpenAI(t *testing.T) (string, func() [][]string) {
+	t.Helper()
+	var (
+		mu    sync.Mutex
+		calls [][]string
+	)
+	url := chattest.NewOpenAI(t, func(req *chattest.OpenAIRequest) chattest.OpenAIResponse {
+		if !req.Stream {
+			return chattest.OpenAINonStreamingResponse("title")
+		}
+		names := make([]string, 0, len(req.Tools))
+		for _, tool := range req.Tools {
+			names = append(names, tool.Function.Name)
+		}
+		mu.Lock()
+		calls = append(calls, names)
+		mu.Unlock()
+		return chattest.OpenAIStreamingResponse(chattest.OpenAITextChunks("ok")...)
+	})
+	recorded := func() [][]string {
+		mu.Lock()
+		defer mu.Unlock()
+		out := make([][]string, len(calls))
+		copy(out, calls)
+		return out
+	}
+	return url, recorded
+}
+
+// TestCreateChat_ForceOnMCPServerEnforced reproduces CDM-02-010 for
+// chat creation: stripping mcp_server_ids from the create request must
+// not exclude force_on MCP servers.
+func TestCreateChat_ForceOnMCPServerEnforced(t *testing.T) {
+	t.Parallel()
+
+	db, ps := dbtestutil.NewDB(t)
+	ctx := testutil.Context(t, testutil.WaitLong)
+
+	forcedURL := newEchoMCPTestServer(t, "forced-mcp")
+	openAIURL, recordedCalls := newToolRecordingOpenAI(t)
+
+	user, org, model := seedChatDependenciesWithProvider(t, db, "openai-compat", openAIURL)
+
+	forcedConfig := dbgen.MCPServerConfig(t, db, database.MCPServerConfig{
+		DisplayName:  "Forced MCP",
+		Slug:         "forced-mcp",
+		Url:          forcedURL,
+		Availability: "force_on",
+		CreatedBy:    uuid.NullUUID{UUID: user.ID, Valid: true},
+		UpdatedBy:    uuid.NullUUID{UUID: user.ID, Valid: true},
+	})
+
+	server := newActiveTestServer(t, db, ps, func(cfg *chatd.Config) {
+		cfg.AIBridgeTransportFactory = chatAIGatewayTransportFactoryPointer(chattest.NewMockAIBridgeTransport(t, openAIURL))
+	})
+
+	// The attacker strips every MCP server ID from the request.
+	chat, err := server.CreateChat(ctx, chatd.CreateOptions{
+		OrganizationID: org.ID,
+		OwnerID:        user.ID,
+		Title:          "forced-mcp-create",
+		ModelConfigID:  model.ID,
+		MCPServerIDs:   []uuid.UUID{},
+		InitialUserContent: []codersdk.ChatMessagePart{
+			codersdk.ChatMessageText("hello"),
+		},
+	})
+	require.NoError(t, err)
+
+	// The force_on server must be persisted despite the empty list.
+	dbChat, err := db.GetChatByID(ctx, chat.ID)
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{forcedConfig.ID}, dbChat.MCPServerIDs,
+		"force_on MCP server must be enforced on chat creation")
+
+	waitForChatProcessed(ctx, t, db, chat.ID, server)
+
+	chatResult, err := db.GetChatByID(ctx, chat.ID)
+	require.NoError(t, err)
+	if chatResult.Status == database.ChatStatusError {
+		require.FailNowf(t, "chat failed", "last_error=%q", chatLastErrorMessage(chatResult.LastError))
+	}
+
+	// The forced server's tool must be offered to the LLM.
+	calls := recordedCalls()
+	require.NotEmpty(t, calls)
+	require.Contains(t, calls[0], "forced-mcp__echo",
+		"force_on MCP tools must be offered to the LLM despite a stripped mcp_server_ids list")
+}
+
+// TestSendMessage_ForceOnMCPServerEnforced reproduces CDM-02-010 for
+// message sends: a tampered mcp_server_ids update must not remove
+// force_on MCP servers from the chat.
+func TestSendMessage_ForceOnMCPServerEnforced(t *testing.T) {
+	t.Parallel()
+
+	db, ps := dbtestutil.NewDB(t)
+	ctx := testutil.Context(t, testutil.WaitLong)
+
+	forcedURL := newEchoMCPTestServer(t, "forced-mcp")
+	optionalURL := newEchoMCPTestServer(t, "optional-mcp")
+	openAIURL, _ := newToolRecordingOpenAI(t)
+
+	user, org, model := seedChatDependenciesWithProvider(t, db, "openai-compat", openAIURL)
+
+	forcedConfig := dbgen.MCPServerConfig(t, db, database.MCPServerConfig{
+		DisplayName:  "Forced MCP",
+		Slug:         "forced-mcp",
+		Url:          forcedURL,
+		Availability: "force_on",
+		CreatedBy:    uuid.NullUUID{UUID: user.ID, Valid: true},
+		UpdatedBy:    uuid.NullUUID{UUID: user.ID, Valid: true},
+	})
+	optionalConfig := dbgen.MCPServerConfig(t, db, database.MCPServerConfig{
+		DisplayName: "Optional MCP",
+		Slug:        "optional-mcp",
+		Url:         optionalURL,
+		CreatedBy:   uuid.NullUUID{UUID: user.ID, Valid: true},
+		UpdatedBy:   uuid.NullUUID{UUID: user.ID, Valid: true},
+	})
+
+	server := newActiveTestServer(t, db, ps, func(cfg *chatd.Config) {
+		cfg.AIBridgeTransportFactory = chatAIGatewayTransportFactoryPointer(chattest.NewMockAIBridgeTransport(t, openAIURL))
+	})
+
+	// Creation with a tampered list that omits the forced server.
+	chat, err := server.CreateChat(ctx, chatd.CreateOptions{
+		OrganizationID: org.ID,
+		OwnerID:        user.ID,
+		Title:          "forced-mcp-send",
+		ModelConfigID:  model.ID,
+		MCPServerIDs:   []uuid.UUID{optionalConfig.ID},
+		InitialUserContent: []codersdk.ChatMessagePart{
+			codersdk.ChatMessageText("hello"),
+		},
+	})
+	require.NoError(t, err)
+
+	dbChat, err := db.GetChatByID(ctx, chat.ID)
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{optionalConfig.ID, forcedConfig.ID}, dbChat.MCPServerIDs,
+		"force_on MCP server must be appended to a tampered create list")
+
+	waitForChatProcessed(ctx, t, db, chat.ID, server)
+
+	// The attacker clears the MCP server list on a message send.
+	_, err = server.SendMessage(ctx, chatd.SendMessageOptions{
+		ChatID:       chat.ID,
+		CreatedBy:    user.ID,
+		Content:      []codersdk.ChatMessagePart{codersdk.ChatMessageText("clear the list")},
+		MCPServerIDs: &[]uuid.UUID{},
+	})
+	require.NoError(t, err)
+
+	dbChat, err = db.GetChatByID(ctx, chat.ID)
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{forcedConfig.ID}, dbChat.MCPServerIDs,
+		"force_on MCP server must survive an emptied mcp_server_ids update")
+
+	waitForChatProcessed(ctx, t, db, chat.ID, server)
+
+	// A legitimate update keeps both the selection and the forced server.
+	_, err = server.SendMessage(ctx, chatd.SendMessageOptions{
+		ChatID:       chat.ID,
+		CreatedBy:    user.ID,
+		Content:      []codersdk.ChatMessagePart{codersdk.ChatMessageText("select optional")},
+		MCPServerIDs: &[]uuid.UUID{optionalConfig.ID},
+	})
+	require.NoError(t, err)
+
+	dbChat, err = db.GetChatByID(ctx, chat.ID)
+	require.NoError(t, err)
+	require.ElementsMatch(t, []uuid.UUID{optionalConfig.ID, forcedConfig.ID}, dbChat.MCPServerIDs,
+		"force_on MCP server must be appended to a tampered update list")
+
+	waitForChatProcessed(ctx, t, db, chat.ID, server)
+}
+
+// TestGeneration_ForceOnMCPServerEnforcedForExistingChats covers chats
+// whose stored mcp_server_ids predates the force_on policy (or was
+// tampered before enforcement existed): generation must still include
+// force_on servers without relying on the stored list.
+func TestGeneration_ForceOnMCPServerEnforcedForExistingChats(t *testing.T) {
+	t.Parallel()
+
+	db, ps := dbtestutil.NewDB(t)
+	ctx := testutil.Context(t, testutil.WaitLong)
+
+	forcedURL := newEchoMCPTestServer(t, "forced-mcp")
+	openAIURL, recordedCalls := newToolRecordingOpenAI(t)
+
+	user, org, model := seedChatDependenciesWithProvider(t, db, "openai-compat", openAIURL)
+
+	server := newActiveTestServer(t, db, ps, func(cfg *chatd.Config) {
+		cfg.AIBridgeTransportFactory = chatAIGatewayTransportFactoryPointer(chattest.NewMockAIBridgeTransport(t, openAIURL))
+	})
+
+	// The chat is created before any force_on MCP server exists, so
+	// its stored mcp_server_ids is empty.
+	chat, err := server.CreateChat(ctx, chatd.CreateOptions{
+		OrganizationID: org.ID,
+		OwnerID:        user.ID,
+		Title:          "forced-mcp-existing",
+		ModelConfigID:  model.ID,
+		MCPServerIDs:   []uuid.UUID{},
+		InitialUserContent: []codersdk.ChatMessagePart{
+			codersdk.ChatMessageText("hello"),
+		},
+	})
+	require.NoError(t, err)
+	waitForChatProcessed(ctx, t, db, chat.ID, server)
+
+	// An admin marks a server force_on after the chat already exists.
+	dbgen.MCPServerConfig(t, db, database.MCPServerConfig{
+		DisplayName:  "Forced MCP",
+		Slug:         "forced-mcp",
+		Url:          forcedURL,
+		Availability: "force_on",
+		CreatedBy:    uuid.NullUUID{UUID: user.ID, Valid: true},
+		UpdatedBy:    uuid.NullUUID{UUID: user.ID, Valid: true},
+	})
+
+	// A send that does not touch mcp_server_ids must still pick up
+	// the force_on server at generation time.
+	_, err = server.SendMessage(ctx, chatd.SendMessageOptions{
+		ChatID:    chat.ID,
+		CreatedBy: user.ID,
+		Content:   []codersdk.ChatMessagePart{codersdk.ChatMessageText("again")},
+	})
+	require.NoError(t, err)
+	waitForChatProcessed(ctx, t, db, chat.ID, server)
+
+	chatResult, err := db.GetChatByID(ctx, chat.ID)
+	require.NoError(t, err)
+	if chatResult.Status == database.ChatStatusError {
+		require.FailNowf(t, "chat failed", "last_error=%q", chatLastErrorMessage(chatResult.LastError))
+	}
+
+	// nil MCPServerIDs must keep the stored list untouched.
+	require.Empty(t, chatResult.MCPServerIDs)
+
+	calls := recordedCalls()
+	require.GreaterOrEqual(t, len(calls), 2)
+	require.NotContains(t, calls[0], "forced-mcp__echo",
+		"no force_on server existed during the first turn")
+	require.Contains(t, calls[len(calls)-1], "forced-mcp__echo",
+		"force_on MCP tools must reach generation for chats created before the policy")
+}
@@ -25,6 +25,53 @@ import (
 	"github.com/coder/coder/v2/codersdk"
 )

+// effectiveMCPServerConfigs loads the MCP server configs for a turn:
+// the chat's stored selection plus every enabled Force On config.
+// Force On inclusion is enforced at generation time, not just at
+// write time, so chats persisted before enforcement existed (or
+// before an admin marked a server Force On) cannot dodge the policy
+// (Cure53 CDM-02-010). Explore chats are exempt: their spawn-time
+// snapshot is immutable by design and must never widen after spawn;
+// Force On servers reach the snapshot through the parent chat's
+// enforced ID list.
+func (server *Server) effectiveMCPServerConfigs(
+	ctx context.Context,
+	logger slog.Logger,
+	chat database.Chat,
+) ([]database.MCPServerConfig, error) {
+	var configs []database.MCPServerConfig
+	if len(chat.MCPServerIDs) > 0 {
+		var err error
+		configs, err = server.db.GetMCPServerConfigsByIDs(ctx, chat.MCPServerIDs)
+		if err != nil {
+			// Best-effort for the user-selected set, matching prior
+			// behavior: a load failure degrades the turn rather than
+			// failing it.
+			logger.Warn(ctx, "failed to load MCP server configs", slog.Error(err))
+			configs = nil
+		}
+	}
+	if isExploreSubagentMode(chat.Mode) {
+		return configs, nil
+	}
+	forced, err := server.db.GetForcedMCPServerConfigs(ctx)
+	if err != nil {
+		// Fail closed: running the turn without the forced set would
+		// silently bypass a security policy.
+		return nil, xerrors.Errorf("get forced MCP server configs: %w", err)
+	}
+	seen := make(map[uuid.UUID]struct{}, len(configs))
+	for _, cfg := range configs {
+		seen[cfg.ID] = struct{}{}
+	}
+	for _, cfg := range forced {
+		if _, ok := seen[cfg.ID]; !ok {
+			configs = append(configs, cfg)
+		}
+	}
+	return configs, nil
+}
+
 func (server *Server) prepareGeneration(
 	ctx context.Context,
 	input generationPrepareInput,
@@ -58,24 +105,11 @@ func (server *Server) prepareGeneration(
 		}
 		return nil
 	})
-	if len(chat.MCPServerIDs) > 0 {
-		g.Go(func() error {
-			var err error
-			mcpConfigs, err = server.db.GetMCPServerConfigsByIDs(ctx, chat.MCPServerIDs)
-			if err != nil {
-				logger.Warn(ctx, "failed to load MCP server configs", slog.Error(err))
-			}
-			return nil
-		})
-		g.Go(func() error {
-			var err error
-			mcpTokens, err = server.db.GetMCPServerUserTokensByUserID(ctx, chat.OwnerID)
-			if err != nil {
-				logger.Warn(ctx, "failed to load MCP user tokens", slog.Error(err))
-			}
-			return nil
-		})
-	}
+	g.Go(func() error {
+		var err error
+		mcpConfigs, err = server.effectiveMCPServerConfigs(ctx, logger, chat)
+		return err
+	})
 	if err := g.Wait(); err != nil {
 		return generationPrepared{}, err
 	}
@@ -311,6 +345,11 @@ func (server *Server) prepareGeneration(
 	})
 	if len(mcpConnectConfigs) > 0 {
 		g2.Go(func() error {
+			var tokenErr error
+			mcpTokens, tokenErr = server.db.GetMCPServerUserTokensByUserID(ctx, chat.OwnerID)
+			if tokenErr != nil {
+				logger.Warn(ctx, "failed to load MCP user tokens", slog.Error(tokenErr))
+			}
 			mcpTokens = server.refreshExpiredMCPTokens(ctx, logger, mcpConnectConfigs, mcpTokens)
 			mcpTools, mcpCleanup = mcpclient.ConnectAll(
 				ctx,