From 9fbf25ea00285b1997d9c2e4aae2db89c5ae9271 Mon Sep 17 00:00:00 2001 From: Sbaaoui Idriss <112825897+6ba3i@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:38:36 +0800 Subject: [PATCH] fix(mcp): prevent managed token exposure (#20639) --- .../mcp/connect_an_external_mcp_to_ragflow.md | 2 + internal/agent/component/agent_mcp_test.go | 19 ++- internal/handler/mcp.go | 8 +- internal/handler/mcp_test.go | 5 +- internal/service/mcp.go | 85 +++++++++-- internal/service/mcp_test.go | 133 +++++++++++++++++- web/src/interfaces/request/mcp.ts | 1 + .../user-setting/mcp/edit-mcp-dialog.tsx | 5 +- web/src/services/mcp-server-service.ts | 4 +- 9 files changed, 238 insertions(+), 24 deletions(-) diff --git a/docs/develop/mcp/connect_an_external_mcp_to_ragflow.md b/docs/develop/mcp/connect_an_external_mcp_to_ragflow.md index 34a2eec300..6d21d5629f 100644 --- a/docs/develop/mcp/connect_an_external_mcp_to_ragflow.md +++ b/docs/develop/mcp/connect_an_external_mcp_to_ragflow.md @@ -66,6 +66,8 @@ sse If the MCP server does not require authentication, leave **Authorization Token** empty. The manual form does not expose arbitrary headers, variables, or a description field. For another authentication scheme or custom headers, use a JSON import with a string-to-string `headers` object, as shown in the example below. +When the server supports bearer authentication, keep the URL free of credentials and put the key in **Authorization Token**. RAGFlow's own MCP endpoint uses this header; query-string keys do not authenticate it. Existing third-party query-authentication URLs remain supported because their authentication requirements depend on the server. Ordinary MCP configuration responses mask the authorization token, and unchanged edits and connection tests reuse the saved value server-side. Explicit configuration downloads include credentials so they can be imported again. + Make sure that the path in **URL** matches the endpoint actually exposed by the MCP server. For example, a Streamable HTTP server commonly uses `/mcp`, while an SSE server commonly uses `/sse`. RAGFlow keeps **Save** disabled until the current connection settings have been tested successfully and at least one tool has been discovered. If you change the URL, server type, or authorization token, test the connection again before saving. diff --git a/internal/agent/component/agent_mcp_test.go b/internal/agent/component/agent_mcp_test.go index d1715e4c47..e95a1c07c7 100644 --- a/internal/agent/component/agent_mcp_test.go +++ b/internal/agent/component/agent_mcp_test.go @@ -39,7 +39,14 @@ func TestAgentMCPModelDispatch(t *testing.T) { common.LookupHost = func(string) ([]string, error) { return []string{"8.8.8.8"}, nil } t.Cleanup(func() { common.LookupHost = lookup }) var dispatched atomic.Bool + const authToken = "ragflow-secret" mcp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.RawQuery != "" { + t.Error("MCP authentication unexpectedly appeared in the URL") + } + if got := r.Header.Get("Authorization"); got != "Bearer "+authToken { + t.Errorf("Authorization = %q, want MCP bearer token", got) + } if r.Method == "DELETE" { w.WriteHeader(204) return @@ -71,7 +78,7 @@ func TestAgentMCPModelDispatch(t *testing.T) { pinned := utility.PinnedHTTPClient utility.PinnedHTTPClient = func(string, string, time.Duration) *http.Client { return mcp.Client() } t.Cleanup(func() { utility.PinnedHTTPClient = pinned }) - if err := db.Create(&entity.MCPServer{ID: "mcp", TenantID: "tenant", Name: "route server", URL: mcp.URL, ServerType: utility.TransportStreamableHTTP}).Error; err != nil { + if err := db.Create(&entity.MCPServer{ID: "mcp", TenantID: "tenant", Name: "route server", URL: mcp.URL, ServerType: utility.TransportStreamableHTTP, Headers: entity.JSONMap{"Authorization": "Bearer ${authorization_token}"}, Variables: entity.JSONMap{"authorization_token": authToken}}).Error; err != nil { t.Fatal(err) } state := runtime.NewCanvasState("run", "task") @@ -84,6 +91,14 @@ func TestAgentMCPModelDispatch(t *testing.T) { } var requests atomic.Int32 llm := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Error(err) + return + } + if strings.Contains(string(body), authToken) || strings.Contains(string(body), mcp.URL) { + t.Error("MCP connection credentials leaked into the model request") + } var req struct { Tools []struct { Function struct { @@ -94,7 +109,7 @@ func TestAgentMCPModelDispatch(t *testing.T) { ToolChoice string `json:"tool_choice"` Messages []models.Message `json:"messages"` } - json.NewDecoder(r.Body).Decode(&req) + json.Unmarshal(body, &req) if len(req.Tools) != 1 || req.Tools[0].Function.Name != "route_0" || req.ToolChoice != "auto" { t.Errorf("tools missing from model request: %+v", req) } diff --git a/internal/handler/mcp.go b/internal/handler/mcp.go index f82ff5ca66..6937d23fa1 100644 --- a/internal/handler/mcp.go +++ b/internal/handler/mcp.go @@ -340,7 +340,7 @@ func (h *MCPHandler) ImportMCPServers(c *gin.Context) { // @Param request body service.TestServerRequest true "test parameters" // @Router /api/v1/mcp/servers/{mcp_id}/test [post] func (h *MCPHandler) TestMCPServer(c *gin.Context) { - _, errorCode, errorMessage := GetUser(c) + user, errorCode, errorMessage := GetUser(c) if errorCode != common.CodeSuccess { common.ErrorWithCode(c, errorCode, errorMessage) return @@ -371,7 +371,7 @@ func (h *MCPHandler) TestMCPServer(c *gin.Context) { } ctx := c.Request.Context() - tools, err := h.mcpService.TestServer(ctx, mcpID, &req) + tools, err := h.mcpService.TestServer(ctx, user.ID, mcpID, &req) if mcpErrorResponse(c, err) { return } @@ -390,8 +390,8 @@ func newMCPServerResponse(server *entity.MCPServer) *mcpServerResponse { URL: server.URL, ServerType: server.ServerType, Description: server.Description, - Variables: map[string]interface{}(server.Variables), - Headers: map[string]interface{}(server.Headers), + Variables: map[string]interface{}(service.MCPVariablesForResponse(server.Variables)), + Headers: map[string]interface{}(service.MCPHeadersForResponse(server.Headers)), CreateTime: server.CreateTime, CreateDate: formatMCPServerDate(server.CreateDate), UpdateTime: server.UpdateTime, diff --git a/internal/handler/mcp_test.go b/internal/handler/mcp_test.go index 4348412450..e0258e13a9 100644 --- a/internal/handler/mcp_test.go +++ b/internal/handler/mcp_test.go @@ -44,7 +44,7 @@ func TestNewMCPServerResponsePreservesNullDescriptionAndFormatsDates(t *testing. TenantID: "tenant-id", URL: "https://example.com/mcp", ServerType: "sse", - Variables: entity.JSONMap{"tools": map[string]interface{}{}}, + Variables: entity.JSONMap{"authorization_token": "ragflow-secret", "tools": map[string]interface{}{}}, Headers: entity.JSONMap{"Authorization": "Bearer token"}, BaseModel: entity.BaseModel{ CreateTime: &createTime, @@ -74,6 +74,9 @@ func TestNewMCPServerResponsePreservesNullDescriptionAndFormatsDates(t *testing. if bytes.Contains(payload, []byte(`+08:00`)) { t.Fatalf("payload %s includes timezone in date fields", payload) } + if bytes.Contains(payload, []byte("ragflow-secret")) || !bytes.Contains(payload, []byte(`"authorization_token":"********"`)) { + t.Fatalf("payload %s exposes the MCP authorization token", payload) + } } func TestMCPDetailDataErrorOmitsDataFieldLikePython(t *testing.T) { diff --git a/internal/service/mcp.go b/internal/service/mcp.go index f15a9f8e99..0872bbba56 100644 --- a/internal/service/mcp.go +++ b/internal/service/mcp.go @@ -22,6 +22,7 @@ import ( "encoding/json" "errors" "fmt" + "maps" "strconv" "strings" "time" @@ -40,6 +41,7 @@ const ( mcpServerNameLimit = 255 defaultMCPFetchTimeoutSec = 10 mcpServerDateFormat = "2006-01-02T15:04:05" + maskedMCPSecret = "********" ) // MCPService handles MCP server operations. @@ -163,8 +165,8 @@ func (s *MCPService) CreateMCPServer(ctx context.Context, tenantID string, req C URL: server.URL, ServerType: server.ServerType, Description: server.Description, - Variables: server.Variables, - Headers: server.Headers, + Variables: MCPVariablesForResponse(server.Variables), + Headers: MCPHeadersForResponse(server.Headers), }, common.CodeSuccess, nil } @@ -229,6 +231,51 @@ func jsonMapHeaderValues(values entity.JSONMap) map[string]string { return out } +// MCPVariablesForResponse masks managed authentication while preserving tool metadata. +func MCPVariablesForResponse(values entity.JSONMap) entity.JSONMap { + result := entity.JSONMap{} + for key, value := range values { + result[key] = value + } + if value, exists := result["authorization_token"]; exists { + if token, ok := jsonValueString(value); ok && token != "" { + result["authorization_token"] = maskedMCPSecret + } + } + return result +} + +// MCPHeadersForResponse masks all nonempty header values because custom headers +// can carry credentials under arbitrary names. +func MCPHeadersForResponse(values entity.JSONMap) entity.JSONMap { + result := entity.JSONMap{} + for key, value := range values { + if text, ok := value.(string); ok && text != "" { + value = maskedMCPSecret + } + result[key] = value + } + return result +} + +func restoreMCPMaskedCredentials(server *entity.MCPServer, url string, headers, variables entity.JSONMap) error { + if variables["authorization_token"] == maskedMCPSecret { + if url != server.URL { + return fmt.Errorf("%w: provide explicit credentials when changing the MCP URL", ErrMCPInvalidURL) + } + variables["authorization_token"] = server.Variables["authorization_token"] + } + for key, value := range headers { + if value == maskedMCPSecret { + if url != server.URL { + return fmt.Errorf("%w: provide explicit headers when changing the MCP URL", ErrMCPInvalidURL) + } + headers[key] = server.Headers[key] + } + } + return nil +} + // jsonValueString renders the scalar JSON values that can occur in an MCP // variable map. Python's string.Template.safe_substitute stringifies these // values before replacing a placeholder; the original values remain in the @@ -390,7 +437,7 @@ func (s *MCPService) UpdateMCPServer(ctx context.Context, tenantID, mcpID string return nil, common.CodeDataError, errors.New("invalid url") } - headers := server.Headers + headers := MCPHeadersForResponse(server.Headers) if raw, ok := req["headers"]; ok { headers = safeJSONMap(raw) } @@ -398,13 +445,16 @@ func (s *MCPService) UpdateMCPServer(ctx context.Context, tenantID, mcpID string headers = entity.JSONMap{} } - variables := server.Variables + variables := MCPVariablesForResponse(server.Variables) if raw, ok := req["variables"]; ok { variables = safeJSONMap(raw) } if variables == nil { variables = entity.JSONMap{} } + if err := restoreMCPMaskedCredentials(server, serverURL, headers, variables); err != nil { + return nil, common.CodeDataError, err + } existingTools := server.Variables["tools"] delete(variables, "tools") needsRefresh := serverURLProvided || serverTypeProvided || headerOrVariablesChanged(req) @@ -503,7 +553,7 @@ func (s *MCPService) ListMCPServers(ctx context.Context, tenantID string, ids [] ServerType: server.ServerType, URL: server.URL, Description: server.Description, - Variables: variables, + Variables: MCPVariablesForResponse(variables), CreateDate: formatMCPServerDate(server.CreateDate), UpdateDate: formatMCPServerDate(server.UpdateDate), }) @@ -741,10 +791,8 @@ func (s *MCPService) nextAvailableMCPName(ctx context.Context, base, tenantID st } } -// TestServerRequest is the body of POST /mcp/servers/:mcp_id/test. The mcp_id -// from the URL path is threaded through to the connect call for log -// correlation; the connection itself is opened from the request body so the -// user can preview unsaved edits — matching Python's test_mcp. +// TestServerRequest previews unsaved connection settings. A masked token is +// resolved from the tenant-owned server identified by the request path. type TestServerRequest struct { URL string `json:"url"` ServerType string `json:"server_type"` @@ -754,13 +802,30 @@ type TestServerRequest struct { } // TestServer opens a live MCP session and returns the tools the server advertises. -func (s *MCPService) TestServer(ctx context.Context, mcpID string, req *TestServerRequest) ([]map[string]interface{}, error) { +func (s *MCPService) TestServer(ctx context.Context, tenantID, mcpID string, req *TestServerRequest) ([]map[string]interface{}, error) { if req == nil || req.URL == "" { return nil, fmt.Errorf("%w: Invalid MCP url", ErrMCPInvalidURL) } if !isValidMCPServerType(req.ServerType) { return nil, ErrMCPInvalidType } + hasMask := req.Variables["authorization_token"] == maskedMCPSecret + for _, value := range req.Headers { + hasMask = hasMask || value == maskedMCPSecret + } + if hasMask { + server, _, err := s.GetMCPServer(ctx, tenantID, mcpID) + if err != nil { + return nil, err + } + preview := *req + preview.Variables = maps.Clone(req.Variables) + preview.Headers = maps.Clone(req.Headers) + req = &preview + if err := restoreMCPMaskedCredentials(server, req.URL, req.Headers, req.Variables); err != nil { + return nil, err + } + } // Run the SSRF guard up front so URL-shape failures (disallowed // scheme, missing host, non-public address) surface as diff --git a/internal/service/mcp_test.go b/internal/service/mcp_test.go index 8acd7d2f74..4e6fd15c28 100644 --- a/internal/service/mcp_test.go +++ b/internal/service/mcp_test.go @@ -99,17 +99,17 @@ func TestServerInputValidation(t *testing.T) { ctx := t.Context() // Empty URL is rejected before any connection attempt. - if _, err := s.TestServer(ctx, "id-1", &TestServerRequest{ServerType: mcpServerTypeSSE}); !errors.Is(err, ErrMCPInvalidURL) { + if _, err := s.TestServer(ctx, "tenant-1", "id-1", &TestServerRequest{ServerType: mcpServerTypeSSE}); !errors.Is(err, ErrMCPInvalidURL) { t.Errorf("expected ErrMCPInvalidURL for empty url, got %v", err) } // nil body is treated as empty URL. - if _, err := s.TestServer(ctx, "id-1", nil); !errors.Is(err, ErrMCPInvalidURL) { + if _, err := s.TestServer(ctx, "tenant-1", "id-1", nil); !errors.Is(err, ErrMCPInvalidURL) { t.Errorf("expected ErrMCPInvalidURL for nil body, got %v", err) } // Invalid server type is rejected before connecting. - if _, err := s.TestServer(ctx, "id-1", &TestServerRequest{URL: "http://example.com/sse", ServerType: "stdio"}); !errors.Is(err, ErrMCPInvalidType) { + if _, err := s.TestServer(ctx, "tenant-1", "id-1", &TestServerRequest{URL: "http://example.com/sse", ServerType: "stdio"}); !errors.Is(err, ErrMCPInvalidType) { t.Errorf("expected ErrMCPInvalidType for bad type, got %v", err) } } @@ -174,6 +174,133 @@ func TestJSONMapStringValuesRendersScalarVariables(t *testing.T) { } } +func TestMCPVariablesForResponseRedactsAuthorizationToken(t *testing.T) { + stored := entity.JSONMap{"authorization_token": "ragflow-secret", "region": "us"} + got := MCPVariablesForResponse(stored) + if got["authorization_token"] != maskedMCPSecret || got["region"] != "us" { + t.Fatalf("response variables = %#v", got) + } + if stored["authorization_token"] != "ragflow-secret" { + t.Fatalf("stored variables were mutated: %#v", stored) + } +} + +func TestMCPResponseMasksScalarTokensAndHeaders(t *testing.T) { + for _, token := range []any{"secret", json.Number("123"), float64(123), true, nil} { + if got := MCPVariablesForResponse(entity.JSONMap{"authorization_token": token}); got["authorization_token"] != maskedMCPSecret { + t.Errorf("token %v was not masked", token) + } + } + headers := entity.JSONMap{"Authorization": "Bearer secret", "X-Custom": "custom-secret", "Empty": ""} + got := MCPHeadersForResponse(headers) + if got["Authorization"] != maskedMCPSecret || got["X-Custom"] != maskedMCPSecret || got["Empty"] != "" || headers["Authorization"] != "Bearer secret" { + t.Fatalf("header masking mutated or exposed credentials: %#v", got) + } +} + +func TestMCPManagedTokenLifecycle(t *testing.T) { + db := setupServiceTestDB(t) + if err := db.AutoMigrate(&entity.MCPServer{}); err != nil { + t.Fatal(err) + } + pushServiceDB(t, db) + if err := db.Create(&entity.Tenant{ID: "tenant-1"}).Error; err != nil { + t.Fatal(err) + } + server, captured := newMCPDiscoveryTestServer(t) + defer server.Close() + withMCPDiscoveryOverrides(t, server) + s := NewMCPService() + created, code, err := s.CreateMCPServer(t.Context(), "tenant-1", CreateMCPServerRequest{ + Name: "managed-auth", URL: server.URL, ServerType: mcpServerTypeStreamableHTTP, + Headers: json.RawMessage(`{"Authorization":"Bearer ${authorization_token}"}`), + Variables: json.RawMessage(`{"authorization_token":"ragflow-secret"}`), + }) + if err != nil || code != common.CodeSuccess { + t.Fatalf("create: code=%d err=%v", code, err) + } + if created.URL != server.URL || created.Variables["authorization_token"] != maskedMCPSecret { + t.Fatalf("create response exposed authentication: %#v", created) + } + if created.Headers["Authorization"] != maskedMCPSecret { + t.Fatal("create response exposed headers") + } + listed, _, err := s.ListMCPServers(t.Context(), "tenant-1", nil, "", 0, 0, "create_time", false) + if err != nil || len(listed.MCPServers) != 1 || listed.MCPServers[0].Variables["authorization_token"] != maskedMCPSecret { + t.Fatalf("list: result=%#v err=%v", listed, err) + } + testReq := &TestServerRequest{URL: server.URL, ServerType: mcpServerTypeStreamableHTTP, Headers: map[string]any(created.Headers), Variables: map[string]any{"authorization_token": maskedMCPSecret}} + beforeRejected := len(captured()) + testReq.URL = server.URL + "/other" + if _, err := s.TestServer(t.Context(), "tenant-1", created.ID, testReq); err == nil { + t.Fatal("preview reused credentials for a different URL") + } + testReq.Variables["authorization_token"] = "replacement" + if _, err := s.TestServer(t.Context(), "tenant-1", created.ID, testReq); err == nil { + t.Fatal("preview reused masked headers for a different URL") + } + testReq.Variables["authorization_token"] = maskedMCPSecret + testReq.URL = server.URL + for _, extra := range []UpdateMCPServerRequest{ + {}, + {"variables": json.RawMessage(`{"authorization_token":"********"}`)}, + {"variables": json.RawMessage(`{"authorization_token":"replacement"}`), "headers": json.RawMessage(`{"Authorization":"********"}`)}, + } { + extra["url"] = json.RawMessage(fmt.Sprintf("%q", server.URL+"/other")) + if _, _, err := s.UpdateMCPServer(t.Context(), "tenant-1", created.ID, extra); err == nil { + t.Fatal("update reused credentials for a different URL") + } + } + if len(captured()) != beforeRejected { + t.Fatal("rejected endpoint changes made outbound requests") + } + if _, err := s.TestServer(t.Context(), "other-tenant", created.ID, testReq); err == nil { + t.Fatal("masked preview resolved another tenant's credential") + } + if _, err := s.TestServer(t.Context(), "tenant-1", created.ID, testReq); err != nil { + t.Fatalf("masked preview: %v", err) + } + if testReq.Variables["authorization_token"] != maskedMCPSecret || testReq.Headers["Authorization"] != maskedMCPSecret { + t.Fatal("preview mutated the client payload") + } + updated, code, err := s.UpdateMCPServer(t.Context(), "tenant-1", created.ID, UpdateMCPServerRequest{ + "url": json.RawMessage(fmt.Sprintf("%q", server.URL)), + "variables": json.RawMessage(`{"authorization_token":"********"}`), + "headers": json.RawMessage(`{"Authorization":"********"}`), + }) + if err != nil || code != common.CodeSuccess || updated.URL != server.URL || updated.Variables["authorization_token"] != "ragflow-secret" { + t.Fatalf("masked update: result=%#v code=%d err=%v", updated, code, err) + } + stored, _, err := s.GetMCPServer(t.Context(), "tenant-1", created.ID) + if err != nil || stored.URL != server.URL || stored.Variables["authorization_token"] != "ragflow-secret" { + t.Fatalf("reload: result=%#v err=%v", stored, err) + } + if stored.Headers["Authorization"] != "Bearer ${authorization_token}" { + t.Fatal("masked update overwrote stored headers") + } + for _, headers := range captured() { + if headers.Get("Authorization") != "Bearer ragflow-secret" { + t.Fatalf("authentication not preserved: %#v", headers) + } + } + exported, _, err := s.ExportMCPServer(t.Context(), "tenant-1", created.ID) + if err != nil { + t.Fatal(err) + } + payload, err := json.Marshal(exported) + if err != nil || !strings.Contains(string(payload), "ragflow-secret") { + t.Fatalf("explicit credential export: payload=%s err=%v", payload, err) + } + updated, code, err = s.UpdateMCPServer(t.Context(), "tenant-1", created.ID, UpdateMCPServerRequest{ + "url": json.RawMessage(fmt.Sprintf("%q", server.URL+"/other")), + "variables": json.RawMessage(`{"authorization_token":"replacement"}`), + "headers": json.RawMessage(`{"Authorization":"Bearer ${authorization_token}"}`), + }) + if err != nil || code != common.CodeSuccess || updated.URL != server.URL+"/other" { + t.Fatalf("explicit credentials must allow endpoint changes: code=%d err=%v", code, err) + } +} + func TestImportServersPreservesExplicitHeadersAndVariables(t *testing.T) { for _, tc := range []struct { name string diff --git a/web/src/interfaces/request/mcp.ts b/web/src/interfaces/request/mcp.ts index 96891ad72a..4df00c6cb1 100644 --- a/web/src/interfaces/request/mcp.ts +++ b/web/src/interfaces/request/mcp.ts @@ -1,6 +1,7 @@ import { IExportedMcpServer } from '@/interfaces/database/mcp'; export interface ITestMcpRequestBody { + mcp_id?: string; server_type: string; url: string; headers?: Record; diff --git a/web/src/pages/user-setting/mcp/edit-mcp-dialog.tsx b/web/src/pages/user-setting/mcp/edit-mcp-dialog.tsx index 20df6410ae..bd9eb6aa3e 100644 --- a/web/src/pages/user-setting/mcp/edit-mcp-dialog.tsx +++ b/web/src/pages/user-setting/mcp/edit-mcp-dialog.tsx @@ -100,8 +100,7 @@ export function EditMcpDialog({ }, []); const handleOk = async (values: z.infer) => { - // When the field still holds the mask, the user did not change the token; - // send the previously stored token so it is not overwritten with empty. + // A masked token is preserved by the backend when editing a saved server. const nextValues = { ...omit(values, 'authorization_token'), ...connectionFields(values.authorization_token, data), @@ -109,7 +108,7 @@ export function EditMcpDialog({ if (isTriggeredBySaving) { onOk?.(nextValues); } else { - const ret = await testMcpServer(nextValues); + const ret = await testMcpServer({ ...nextValues, mcp_id: id }); if (ret.code === 0) { setFieldChanged(false); } diff --git a/web/src/services/mcp-server-service.ts b/web/src/services/mcp-server-service.ts index 4fcdb28410..f22a49f49d 100644 --- a/web/src/services/mcp-server-service.ts +++ b/web/src/services/mcp-server-service.ts @@ -34,7 +34,9 @@ const mcpServerService = { export: ({ mcp_id }: { mcp_id: string }) => request.get(api.exportMcpServer(mcp_id)), test: (params: Record) => - request.post(api.testMcpServer(params.name || 'preview'), { data: params }), + request.post(api.testMcpServer(params.mcp_id || params.name || 'preview'), { + data: params, + }), }; export default mcpServerService;