From 079bb32673c0c72b17f9fa18676780354e6c605b Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 14 Nov 2025 20:35:45 +0000 Subject: [PATCH 1/2] mcp: validate tool names Implement SEP 986, enforcing restrictions on tool names. Since we're already at v1 we couldn't turn this into a panic, so use an error log instead. This is consistent with the typescript SDK. A note is left in our 'rough edges' doc to panic in v2. Fixes #621 --- docs/rough_edges.md | 6 +++ internal/docs/rough_edges.src.md | 6 +++ mcp/server.go | 3 ++ mcp/server_test.go | 65 ++++++++++++++++++++++++++++++++ mcp/shared.go | 2 + mcp/tool.go | 24 ++++++++++++ mcp/tool_test.go | 50 ++++++++++++++++++++++++ 7 files changed, 156 insertions(+) diff --git a/docs/rough_edges.md b/docs/rough_edges.md index 7bb7732c..a99e00a8 100644 --- a/docs/rough_edges.md +++ b/docs/rough_edges.md @@ -10,3 +10,9 @@ v2. of the SDK where event persistence and delivery were combined. **Workaround**: `Open` may be implemented as a no-op. + +- Enforcing valid tool names: with + [SEP-986](https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986) + landing after the SDK was at v1, we missed an opportunity to panic on invalid + tool names. Instead, we have to simply produce an error log. In v2, we should + panic. diff --git a/internal/docs/rough_edges.src.md b/internal/docs/rough_edges.src.md index 150509b4..d8c2c996 100644 --- a/internal/docs/rough_edges.src.md +++ b/internal/docs/rough_edges.src.md @@ -9,3 +9,9 @@ v2. of the SDK where event persistence and delivery were combined. **Workaround**: `Open` may be implemented as a no-op. + +- Enforcing valid tool names: with + [SEP-986](https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986) + landing after the SDK was at v1, we missed an opportunity to panic on invalid + tool names. Instead, we have to simply produce an error log. In v2, we should + panic. diff --git a/mcp/server.go b/mcp/server.go index 4a7bc89a..6a3368e1 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -191,6 +191,9 @@ func (s *Server) RemovePrompts(names ...string) { // Most users should use the top-level function [AddTool], which handles all these // responsibilities. func (s *Server) AddTool(t *Tool, h ToolHandler) { + if err := validateToolName(t.Name); err != nil { + s.opts.Logger.Error(fmt.Sprintf("AddTool: invalid tool name %q: %v", t.Name, err)) + } if t.InputSchema == nil { // This prevents the tool author from forgetting to write a schema where // one should be provided. If we papered over this by supplying the empty diff --git a/mcp/server_test.go b/mcp/server_test.go index ec6114d0..d8c0df65 100644 --- a/mcp/server_test.go +++ b/mcp/server_test.go @@ -5,9 +5,11 @@ package mcp import ( + "bytes" "context" "encoding/json" "log" + "log/slog" "slices" "strings" "testing" @@ -490,6 +492,69 @@ func TestAddTool(t *testing.T) { } } +func TestAddToolNameValidation(t *testing.T) { + tests := []struct { + label string + name string + wantLogContaining string + }{ + { + label: "empty name", + name: "", + wantLogContaining: `tool name cannot be empty`, + }, + { + label: "long name", + name: strings.Repeat("a", 129), + wantLogContaining: "exceeds maximum length of 128 characters", + }, + { + label: "name with spaces", + name: "get user profile", + wantLogContaining: `tool name contains invalid characters: \" \"`, + }, + { + label: "name with multiple invalid chars", + name: "user name@domain,com", + wantLogContaining: `tool name contains invalid characters: \" \", \"@\", \",\"`, + }, + { + label: "name with unicode", + name: "tool-ñame", + wantLogContaining: `tool name contains invalid characters: \"ñ\"`, + }, + { + label: "valid name", + name: "valid-tool_name.123", + wantLogContaining: "", // No log expected + }, + } + for _, test := range tests { + t.Run(test.label, func(t *testing.T) { + var buf bytes.Buffer + s := NewServer(testImpl, &ServerOptions{ + Logger: slog.New(slog.NewTextHandler(&buf, nil)), + }) + + // Use the generic AddTool as it also calls validateToolName. + AddTool(s, &Tool{Name: test.name}, func(context.Context, *CallToolRequest, any) (*CallToolResult, any, error) { + return nil, nil, nil + }) + + logOutput := buf.String() + if test.wantLogContaining != "" { + if !strings.Contains(logOutput, test.wantLogContaining) { + t.Errorf("log output =\n%s\nwant containing %q", logOutput, test.wantLogContaining) + } + } else { + if logOutput != "" { + t.Errorf("expected empty log output, got %q", logOutput) + } + } + }) + } +} + type schema = jsonschema.Schema func testToolForSchema[In, Out any](t *testing.T, tool *Tool, in string, out Out, wantIn, wantOut any, wantErrContaining string) { diff --git a/mcp/shared.go b/mcp/shared.go index e90bcbd8..b710c952 100644 --- a/mcp/shared.go +++ b/mcp/shared.go @@ -34,12 +34,14 @@ const ( // It is the version that the client sends in the initialization request, and // the default version used by the server. latestProtocolVersion = protocolVersion20250618 + protocolVersion20251125 = "2025-11-25" // not yet released protocolVersion20250618 = "2025-06-18" protocolVersion20250326 = "2025-03-26" protocolVersion20241105 = "2024-11-05" ) var supportedProtocolVersions = []string{ + protocolVersion20251125, protocolVersion20250618, protocolVersion20250326, protocolVersion20241105, diff --git a/mcp/tool.go b/mcp/tool.go index 12b02b7b..62b73ea5 100644 --- a/mcp/tool.go +++ b/mcp/tool.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" "fmt" + "strings" "github.com/google/jsonschema-go/jsonschema" ) @@ -101,3 +102,26 @@ func applySchema(data json.RawMessage, resolved *jsonschema.Resolved) (json.RawM } return data, nil } + +func validateToolName(name string) error { + if name == "" { + return fmt.Errorf("tool name cannot be empty") + } + if len(name) > 128 { + return fmt.Errorf("tool name exceeds maximum length of 128 characters (current: %d)", len(name)) + } + var invalidChars []string + seen := make(map[rune]bool) + for _, r := range name { + if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' || r == '-' || r == '.') { + if !seen[r] { + invalidChars = append(invalidChars, fmt.Sprintf("%q", string(r))) + seen[r] = true + } + } + } + if len(invalidChars) > 0 { + return fmt.Errorf("tool name contains invalid characters: %s", strings.Join(invalidChars, ", ")) + } + return nil +} diff --git a/mcp/tool_test.go b/mcp/tool_test.go index c440fcc5..80661d31 100644 --- a/mcp/tool_test.go +++ b/mcp/tool_test.go @@ -145,3 +145,53 @@ func TestToolErrorHandling(t *testing.T) { } }) } + +func TestValidateToolName(t *testing.T) { + t.Run("valid", func(t *testing.T) { + validTests := []struct { + label string + toolName string + }{ + {"simple alphanumeric names", "getUser"}, + {"names with underscores", "get_user_profile"}, + {"names with dashes", "user-profile-update"}, + {"names with dots", "admin.tools.list"}, + {"mixed character names", "DATA_EXPORT_v2.1"}, + {"single character names", "a"}, + {"128 character names", strings.Repeat("a", 128)}, + } + for _, test := range validTests { + t.Run(test.label, func(t *testing.T) { + if err := validateToolName(test.toolName); err != nil { + t.Errorf("validateToolName(%q) = %v, want nil", test.toolName, err) + } + }) + } + }) + + t.Run("invalid", func(t *testing.T) { + invalidTests := []struct { + label string + toolName string + wantErrContaining string + }{ + {"empty names", "", "tool name cannot be empty"}, + {"names longer than 128 characters", strings.Repeat("a", 129), "tool name exceeds maximum length of 128 characters (current: 129)"}, + {"names with spaces", "get user profile", `tool name contains invalid characters: " "`}, + {"names with commas", "get,user,profile", `tool name contains invalid characters: ","`}, + {"names with forward slashes", "user/profile/update", `tool name contains invalid characters: "/"`}, + {"names with other special chars", "user@domain.com", `tool name contains invalid characters: "@"`}, + {"names with multiple invalid chars", "user name@domain,com", `tool name contains invalid characters: " ", "@", ","`}, + {"names with unicode characters", "user-ñame", `tool name contains invalid characters: "ñ"`}, + } + for _, test := range invalidTests { + t.Run(test.label, func(t *testing.T) { + if err := validateToolName(test.toolName); err == nil || !strings.Contains(err.Error(), test.wantErrContaining) { + t.Errorf("validateToolName(%q) = %v, want error containing %q", test.toolName, err, test.wantErrContaining) + } + }) + } + + }) + +} From 058dd225e6cef9ba05b270a1c4e74e40f4175169 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 19 Nov 2025 02:25:25 +0000 Subject: [PATCH 2/2] address review comments --- mcp/tool.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/mcp/tool.go b/mcp/tool.go index 62b73ea5..8aa7c3c0 100644 --- a/mcp/tool.go +++ b/mcp/tool.go @@ -103,6 +103,8 @@ func applySchema(data json.RawMessage, resolved *jsonschema.Resolved) (json.RawM return data, nil } +// validateToolName checks whether name is a valid tool name, reporting a +// non-nil error if not. func validateToolName(name string) error { if name == "" { return fmt.Errorf("tool name cannot be empty") @@ -110,10 +112,12 @@ func validateToolName(name string) error { if len(name) > 128 { return fmt.Errorf("tool name exceeds maximum length of 128 characters (current: %d)", len(name)) } + // For consistency with other SDKs, report characters in the order the appear + // in the name. var invalidChars []string seen := make(map[rune]bool) for _, r := range name { - if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' || r == '-' || r == '.') { + if !validToolNameRune(r) { if !seen[r] { invalidChars = append(invalidChars, fmt.Sprintf("%q", string(r))) seen[r] = true @@ -125,3 +129,11 @@ func validateToolName(name string) error { } return nil } + +// validToolNameRune reports whether r is valid within tool names. +func validToolNameRune(r rune) bool { + return (r >= 'a' && r <= 'z') || + (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || + r == '_' || r == '-' || r == '.' +}