From 9e802e005bb854138dcc8ecad02aa196bd239508 Mon Sep 17 00:00:00 2001 From: Alex Dunmow Date: Mon, 10 Aug 2026 22:49:09 +0800 Subject: [PATCH] fix(bnp): validate plugin MCP manifests --- ...-artifacts-are-validated-before-packing.md | 28 +++ go.mod | 2 +- go.sum | 4 +- internal/bnp/build.go | 5 +- internal/bnp/mcp_tools.go | 160 ++++++++++++++++++ internal/bnp/mcp_tools_test.go | 110 ++++++++++++ internal/bnp/verify.go | 9 +- 7 files changed, 313 insertions(+), 5 deletions(-) create mode 100644 docs/adr/0001-plugin-mcp-artifacts-are-validated-before-packing.md create mode 100644 internal/bnp/mcp_tools.go create mode 100644 internal/bnp/mcp_tools_test.go diff --git a/docs/adr/0001-plugin-mcp-artifacts-are-validated-before-packing.md b/docs/adr/0001-plugin-mcp-artifacts-are-validated-before-packing.md new file mode 100644 index 0000000..c33649a --- /dev/null +++ b/docs/adr/0001-plugin-mcp-artifacts-are-validated-before-packing.md @@ -0,0 +1,28 @@ +# Plugin MCP artifacts are validated before packing + +Plugin SDK v0.3.2 serializes Connect service descriptors into +`PluginManifest.mcp_tools`. Without a CLI gate, an artifact could carry +inconsistent procedures, missing RBAC coverage, invalid schemas, unstable +names, or no runtime HTTP handler and fail only after installation. + +Decision: both `ninja plugin build` and `ninja plugin verify` validate the MCP +surface. Descriptor procedures must agree with service and method fields, be +unique, map to supported RBAC roles, contain object input schemas, and derive a +scoped name no longer than 128 bytes. The protobuf package must end in an API +major such as `v1`, `plugin.mod` must provide scope and plugin identity, and any +descriptor-bearing manifest must set `has_http_handler`. The CLI pins Plugin +SDK v0.3.2 so builds preserve the typed descriptor field. + +Relying only on the CMS loader was rejected because it lets known-invalid +artifacts reach the registry. Validating only during build was rejected because +`verify` must independently gate externally supplied artifacts. + +Consequences: + +- Invalid MCP artifacts fail before upload or installation. +- The CLI and CMS enforce the same descriptor and runtime invariants. +- Wiki artifacts built with this CLI retain all typed MCP descriptors. + +Keywords: ninja plugin build, ninja plugin verify, Plugin SDK v0.3.2, +MCPToolDescriptor, mcp_tools, has_http_handler, plugin.mod scope, RBAC, JSON +Schema, plugin_ninja_wiki_v1 diff --git a/go.mod b/go.mod index 98a1bc4..89b30b3 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.26.4 require ( connectrpc.com/connect v1.20.0 git.dev.alexdunmow.com/block/core v0.18.2 - git.dev.alexdunmow.com/block/pluginsdk v0.2.7 + git.dev.alexdunmow.com/block/pluginsdk v0.3.2 github.com/chromedp/cdproto v0.0.0-20260321001828-e3e3800016bc github.com/chromedp/chromedp v0.15.1 github.com/klauspost/compress v1.18.6 diff --git a/go.sum b/go.sum index 0a6ff53..ea1a254 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,8 @@ connectrpc.com/connect v1.20.0 h1:6TNDAB+WeNd2uolWNlYczB5E0KNNaVMNUEx8JEUsPmQ= connectrpc.com/connect v1.20.0/go.mod h1:A2ygJrukXwWy32vkCAAHNVguZrqZ+jeZ9rGRnGR4dN4= git.dev.alexdunmow.com/block/core v0.18.2 h1:+3OfZ424yoc1k1CucXYARk8TBkh8Z693HRkhf5Ci2wU= git.dev.alexdunmow.com/block/core v0.18.2/go.mod h1:GGuUu826AoJepC/hKLGJ7BX3PQaDss9ueCT0se6Ao2w= -git.dev.alexdunmow.com/block/pluginsdk v0.2.7 h1:iL6Qvg2xHqHtii/0ZF9TIjjDzMrYkmxvLwqU300nmac= -git.dev.alexdunmow.com/block/pluginsdk v0.2.7/go.mod h1:Z+eG+WZxAP0jfreLqlGcc0kkWKt8RWevzWyWn8d+dhM= +git.dev.alexdunmow.com/block/pluginsdk v0.3.2 h1:aLGkLzmJ0+docoke1e9sZTtugNugPaD3PNJEonzurgI= +git.dev.alexdunmow.com/block/pluginsdk v0.3.2/go.mod h1:Z+eG+WZxAP0jfreLqlGcc0kkWKt8RWevzWyWn8d+dhM= github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk= github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= github.com/chromedp/cdproto v0.0.0-20260321001828-e3e3800016bc h1:wkN/LMi5vc60pBRWx6qpbk/aEvq3/ZVNpnMvsw8PVVU= diff --git a/internal/bnp/build.go b/internal/bnp/build.go index ebf24c4..630cffc 100644 --- a/internal/bnp/build.go +++ b/internal/bnp/build.go @@ -12,8 +12,8 @@ import ( "strconv" abiv1 "git.dev.alexdunmow.com/block/pluginsdk/abi/v1" - core "git.dev.alexdunmow.com/block/pluginsdk/plugin" "git.dev.alexdunmow.com/block/pluginsdk/egress" + core "git.dev.alexdunmow.com/block/pluginsdk/plugin" "google.golang.org/protobuf/proto" ) @@ -165,6 +165,9 @@ func Build(ctx context.Context, opts BuildOptions) (*BuildResult, error) { if err := applyManifestYAML(dir, manifest); err != nil { return nil, err } + if err := ValidateMCPManifest(manifest, mod.Plugin.Scope, mod.Plugin.Name); err != nil { + return nil, err + } manifestBytes, err := proto.Marshal(manifest) if err != nil { diff --git a/internal/bnp/mcp_tools.go b/internal/bnp/mcp_tools.go new file mode 100644 index 0000000..ae1e7fa --- /dev/null +++ b/internal/bnp/mcp_tools.go @@ -0,0 +1,160 @@ +package bnp + +import ( + "encoding/json" + "fmt" + "strings" + "unicode" + + abiv1 "git.dev.alexdunmow.com/block/pluginsdk/abi/v1" +) + +// ValidateMCPManifest enforces the descriptor/runtime contract shared with the +// CMS before an artifact can be packed or accepted by `ninja plugin verify`. +func ValidateMCPManifest(manifest *abiv1.PluginManifest, registryScope, pluginName string) error { + descriptors := manifest.GetMcpTools() + if len(descriptors) == 0 { + return nil + } + if !manifest.GetHasHttpHandler() { + return fmt.Errorf("bnp: MCP descriptors require has_http_handler so tools are callable at runtime") + } + + seenProcedures := make(map[string]bool, len(descriptors)) + seenToolNames := make(map[string]bool, len(descriptors)) + for _, descriptor := range descriptors { + if descriptor == nil { + return fmt.Errorf("bnp: MCP descriptor is nil") + } + procedure := strings.TrimSpace(descriptor.GetProcedure()) + serviceName, methodName, ok := splitConnectProcedure(procedure) + if !ok || serviceName != descriptor.GetServiceFullName() || methodName != descriptor.GetMethodName() { + return fmt.Errorf("bnp: MCP descriptor has inconsistent procedure %q", procedure) + } + if seenProcedures[procedure] { + return fmt.Errorf("bnp: duplicate MCP procedure %q", procedure) + } + seenProcedures[procedure] = true + + role, mapped := manifest.GetRbacMethodRoles()[procedure] + if !mapped || !supportedMCPRole(role) { + return fmt.Errorf("bnp: MCP procedure %q has no supported RBAC role", procedure) + } + if !validMCPObjectSchema(descriptor.GetInputSchemaJson()) { + return fmt.Errorf("bnp: MCP procedure %q has an invalid input schema", procedure) + } + + toolName, err := mcpToolName(registryScope, pluginName, serviceName, methodName) + if err != nil { + return fmt.Errorf("bnp: MCP procedure %q: %w", procedure, err) + } + if seenToolNames[toolName] { + return fmt.Errorf("bnp: MCP procedures produce duplicate tool name %q", toolName) + } + seenToolNames[toolName] = true + } + return nil +} + +func supportedMCPRole(role string) bool { + switch role { + case "", "public", "viewer", "admin", "superadmin": + return true + default: + return false + } +} + +func validMCPObjectSchema(raw []byte) bool { + var schema map[string]any + if json.Unmarshal(raw, &schema) != nil { + return false + } + typeName, _ := schema["type"].(string) + return typeName == "object" +} + +func splitConnectProcedure(procedure string) (serviceName, methodName string, ok bool) { + trimmed := strings.TrimPrefix(strings.TrimSpace(procedure), "/") + serviceName, methodName, ok = strings.Cut(trimmed, "/") + return serviceName, methodName, ok && serviceName != "" && methodName != "" && !strings.Contains(methodName, "/") +} + +func mcpToolName(registryScope, pluginName, serviceFullName, methodName string) (string, error) { + scope := mcpIdentifier(strings.TrimPrefix(strings.TrimSpace(registryScope), "@")) + plugin := mcpIdentifier(pluginName) + if scope == "" || plugin == "" { + return "", fmt.Errorf("registry scope and plugin name must be non-empty MCP identifiers") + } + + parts := strings.Split(serviceFullName, ".") + if len(parts) < 2 { + return "", fmt.Errorf("service %q has no protobuf package", serviceFullName) + } + apiMajor := parts[len(parts)-2] + if !validAPIMajor(apiMajor) { + return "", fmt.Errorf("service %q protobuf package must end in an API major such as v1", serviceFullName) + } + service := mcpIdentifier(strings.TrimSuffix(parts[len(parts)-1], "Service")) + if service == "" { + return "", fmt.Errorf("service %q has no service qualifier", serviceFullName) + } + + domain := "plugin_" + scope + "_" + plugin + "_" + apiMajor + if service != plugin { + domain += "_" + service + } + toolName := domain + "_" + mcpIdentifier(methodName) + if len(toolName) > 128 { + return "", fmt.Errorf("tool name %q exceeds MCP's 128-byte limit", toolName) + } + return toolName, nil +} + +func validAPIMajor(value string) bool { + if len(value) < 2 || value[0] != 'v' { + return false + } + for _, character := range value[1:] { + if character < '0' || character > '9' { + return false + } + } + return true +} + +func mcpIdentifier(value string) string { + value = snakeIdentifier(strings.TrimSpace(value)) + var builder strings.Builder + previousSeparator := false + for _, character := range value { + if (character >= 'a' && character <= 'z') || (character >= '0' && character <= '9') { + builder.WriteRune(character) + previousSeparator = false + continue + } + if builder.Len() > 0 && !previousSeparator { + builder.WriteByte('_') + previousSeparator = true + } + } + return strings.TrimSuffix(builder.String(), "_") +} + +func snakeIdentifier(value string) string { + var builder strings.Builder + runes := []rune(value) + for index, current := range runes { + if unicode.IsUpper(current) { + previousLower := index > 0 && unicode.IsLower(runes[index-1]) + nextLower := index+1 < len(runes) && unicode.IsLower(runes[index+1]) + if index > 0 && (previousLower || nextLower) { + builder.WriteByte('_') + } + builder.WriteRune(unicode.ToLower(current)) + continue + } + builder.WriteRune(current) + } + return builder.String() +} diff --git a/internal/bnp/mcp_tools_test.go b/internal/bnp/mcp_tools_test.go new file mode 100644 index 0000000..be10c1d --- /dev/null +++ b/internal/bnp/mcp_tools_test.go @@ -0,0 +1,110 @@ +package bnp + +import ( + "path/filepath" + "strings" + "testing" + + abiv1 "git.dev.alexdunmow.com/block/pluginsdk/abi/v1" + "google.golang.org/protobuf/proto" +) + +func wikiMCPManifest(hasHTTPHandler bool) *abiv1.PluginManifest { + procedure := "/wiki.v1.WikiService/SaveArticle" + return &abiv1.PluginManifest{ + AbiVersion: 1, + Name: "wiki", + Version: "0.1.12", + HasHttpHandler: hasHTTPHandler, + RbacMethodRoles: map[string]string{procedure: "admin"}, + McpTools: []*abiv1.MCPToolDescriptor{{ + Procedure: procedure, + ServiceFullName: "wiki.v1.WikiService", + MethodName: "SaveArticle", + InputSchemaJson: []byte(`{"type":"object","properties":{"article":{"type":"object"}}}`), + }}, + } +} + +func TestValidateMCPManifestAcceptsCallableWikiDescriptor(t *testing.T) { + manifest := wikiMCPManifest(true) + if err := ValidateMCPManifest(manifest, "@ninja", "wiki"); err != nil { + t.Fatalf("ValidateMCPManifest() error = %v", err) + } + name, err := mcpToolName("@ninja", "wiki", "wiki.v1.WikiService", "SaveArticle") + if err != nil { + t.Fatalf("mcpToolName() error = %v", err) + } + if name != "plugin_ninja_wiki_v1_save_article" { + t.Fatalf("tool name = %q", name) + } +} + +func TestValidateMCPManifestRejectsMissingRuntimeHandler(t *testing.T) { + err := ValidateMCPManifest(wikiMCPManifest(false), "@ninja", "wiki") + if err == nil || !strings.Contains(err.Error(), "has_http_handler") { + t.Fatalf("error = %v, want has_http_handler rejection", err) + } +} + +func TestValidateMCPManifestRejectsMalformedDescriptors(t *testing.T) { + tests := []struct { + name string + mutate func(*abiv1.PluginManifest) + want string + }{ + { + name: "unmapped procedure", + mutate: func(manifest *abiv1.PluginManifest) { + manifest.RbacMethodRoles = nil + }, + want: "RBAC role", + }, + { + name: "invalid schema", + mutate: func(manifest *abiv1.PluginManifest) { + manifest.McpTools[0].InputSchemaJson = []byte(`{"type":"string"}`) + }, + want: "input schema", + }, + { + name: "missing API major", + mutate: func(manifest *abiv1.PluginManifest) { + descriptor := manifest.McpTools[0] + descriptor.Procedure = "/wiki.WikiService/SaveArticle" + descriptor.ServiceFullName = "wiki.WikiService" + manifest.RbacMethodRoles = map[string]string{descriptor.Procedure: "admin"} + }, + want: "API major", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + manifest := wikiMCPManifest(true) + test.mutate(manifest) + err := ValidateMCPManifest(manifest, "@ninja", "wiki") + if err == nil || !strings.Contains(err.Error(), test.want) { + t.Fatalf("error = %v, want %q", err, test.want) + } + }) + } +} + +func TestVerifyRejectsMCPDescriptorsWithoutRuntimeHandler(t *testing.T) { + manifestBytes, err := proto.Marshal(wikiMCPManifest(false)) + if err != nil { + t.Fatalf("marshal manifest: %v", err) + } + out := filepath.Join(t.TempDir(), "wiki.bnp") + _, err = packArtifact(out, []packEntry{ + {ArtifactPath: fileWasm, Data: []byte("wasm")}, + {ArtifactPath: fileMod, Data: []byte("[plugin]\nname = \"wiki\"\nscope = \"@ninja\"\nversion = \"0.1.12\"\n")}, + {ArtifactPath: fileManifest, Data: manifestBytes}, + }) + if err != nil { + t.Fatalf("pack artifact: %v", err) + } + if _, err := Verify(out); err == nil || !strings.Contains(err.Error(), "has_http_handler") { + t.Fatalf("Verify() error = %v, want has_http_handler rejection", err) + } +} diff --git a/internal/bnp/verify.go b/internal/bnp/verify.go index 1e78750..0c8b128 100644 --- a/internal/bnp/verify.go +++ b/internal/bnp/verify.go @@ -121,6 +121,9 @@ func Verify(bnpPath string) (*VerifyResult, error) { if modName != name { return nil, fmt.Errorf("bnp: manifest name %q != plugin.mod name %q", name, modName) } + if err := ValidateMCPManifest(manifest, parseModString(modBytes, "scope"), name); err != nil { + return nil, err + } return &VerifyResult{ Name: name, @@ -332,9 +335,13 @@ func dirExists(p string) bool { // mirroring the reader's tolerant line scan (works whether or not the key sits // under a [plugin] table). func parseModName(data []byte) string { + return parseModString(data, "name") +} + +func parseModString(data []byte, key string) string { sc := bufio.NewScanner(bytes.NewReader(data)) for sc.Scan() { - after, ok := strings.CutPrefix(strings.TrimSpace(sc.Text()), "name") + after, ok := strings.CutPrefix(strings.TrimSpace(sc.Text()), key) if !ok { continue }