mcp: validate tool names#640
Conversation
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 modelcontextprotocol#621
mcp/tool.go
Outdated
| 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 == '.') { |
There was a problem hiding this comment.
nit: a switch would be clearer
There was a problem hiding this comment.
Hmm, checked the standard library and we don't use switches for this sort of check anywhere: we either use a conditional or boolean array.
Factored out to an (inlined) helper function, which I think makes it clearer. WDYT?
| 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))) |
There was a problem hiding this comment.
Use maps.Keys(seen) at the end.
There was a problem hiding this comment.
Can't: need to report characters in the order they occur. Want to be consistent with other SDKs.
|
@markus-kusano or @samthanawalla could you review? |
| 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)) |
There was a problem hiding this comment.
feel free to ignore: should we include the tool name here to make it easier for debugging? i can also see this being a problem since it is too long. i could see this error message being annoying if the user could have multiple tool names and they don't know which one is too long
There was a problem hiding this comment.
The tool name is included at the call site.
An arguable style choice, but I mildly prefer having validators only describe the specific error, because this then leave it up to the caller for how to present that error. For example the caller could do "Adding tool %q: %v"
4 participants
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