mcp: handle bad or missing params (modelcontextprotocol#210)

Audit all cases where params must not be null, and enforce this using
methodInfo via a new methodFlags bitfield that parameterizes method
requirements. Write extensive conformance tests catching all the
(server-side) crashes that were possible.

We should go further and validate schema against the spec, but that is
more indirect, more complicated, and potentially slower. For now we
adopt this more explicit approach.

Still TODO in a subsequent CL: verify the client side of this with
client conformance tests.

Additionally, improve some error messages that were redundant or leaked
internal implementation details.

Fixes modelcontextprotocol#195

Author: findleyr

internal/jsonrpc2/wire.go

@@ -13,30 +13,30 @@ import (
 
 var (
 	// ErrParse is used when invalid JSON was received by the server.
-	ErrParse = NewError(-32700, "JSON RPC parse error")
+	ErrParse = NewError(-32700, "parse error")
 	// ErrInvalidRequest is used when the JSON sent is not a valid Request object.
-	ErrInvalidRequest = NewError(-32600, "JSON RPC invalid request")
+	ErrInvalidRequest = NewError(-32600, "invalid request")
 	// ErrMethodNotFound should be returned by the handler when the method does
 	// not exist / is not available.
-	ErrMethodNotFound = NewError(-32601, "JSON RPC method not found")
+	ErrMethodNotFound = NewError(-32601, "method not found")
 	// ErrInvalidParams should be returned by the handler when method
 	// parameter(s) were invalid.
-	ErrInvalidParams = NewError(-32602, "JSON RPC invalid params")
+	ErrInvalidParams = NewError(-32602, "invalid params")
 	// ErrInternal indicates a failure to process a call correctly
-	ErrInternal = NewError(-32603, "JSON RPC internal error")
+	ErrInternal = NewError(-32603, "internal error")
 
 	// The following errors are not part of the json specification, but
 	// compliant extensions specific to this implementation.
 
 	// ErrServerOverloaded is returned when a message was refused due to a
 	// server being temporarily unable to accept any new messages.
-	ErrServerOverloaded = NewError(-32000, "JSON RPC overloaded")
+	ErrServerOverloaded = NewError(-32000, "overloaded")
 	// ErrUnknown should be used for all non coded errors.
-	ErrUnknown = NewError(-32001, "JSON RPC unknown error")
+	ErrUnknown = NewError(-32001, "unknown error")
 	// ErrServerClosing is returned for calls that arrive while the server is closing.
-	ErrServerClosing = NewError(-32004, "JSON RPC server is closing")
+	ErrServerClosing = NewError(-32004, "server is closing")
 	// ErrClientClosing is a dummy error returned for calls initiated while the client is closing.
-	ErrClientClosing = NewError(-32003, "JSON RPC client is closing")
+	ErrClientClosing = NewError(-32003, "client is closing")
 )
 
 const wireVersion = "2.0"

mcp/client.go

@@ -286,17 +286,21 @@ func (c *Client) AddReceivingMiddleware(middleware ...Middleware[*ClientSession]
 }
 
 // clientMethodInfos maps from the RPC method name to serverMethodInfos.
+//
+// The 'allowMissingParams' values are extracted from the protocol schema.
+// TODO(rfindley): actually load and validate the protocol schema, rather than
+// curating these method flags.
 var clientMethodInfos = map[string]methodInfo{
-	methodComplete:                  newMethodInfo(sessionMethod((*ClientSession).Complete), true),
-	methodPing:                      newMethodInfo(sessionMethod((*ClientSession).ping), true),
-	methodListRoots:                 newMethodInfo(clientMethod((*Client).listRoots), true),
-	methodCreateMessage:             newMethodInfo(clientMethod((*Client).createMessage), true),
-	notificationToolListChanged:     newMethodInfo(clientMethod((*Client).callToolChangedHandler), false),
-	notificationPromptListChanged:   newMethodInfo(clientMethod((*Client).callPromptChangedHandler), false),
-	notificationResourceListChanged: newMethodInfo(clientMethod((*Client).callResourceChangedHandler), false),
-	notificationResourceUpdated:     newMethodInfo(clientMethod((*Client).callResourceUpdatedHandler), false),
-	notificationLoggingMessage:      newMethodInfo(clientMethod((*Client).callLoggingHandler), false),
-	notificationProgress:            newMethodInfo(sessionMethod((*ClientSession).callProgressNotificationHandler), false),
+	methodComplete:                  newMethodInfo(sessionMethod((*ClientSession).Complete), 0),
+	methodPing:                      newMethodInfo(sessionMethod((*ClientSession).ping), missingParamsOK),
+	methodListRoots:                 newMethodInfo(clientMethod((*Client).listRoots), missingParamsOK),
+	methodCreateMessage:             newMethodInfo(clientMethod((*Client).createMessage), 0),
+	notificationToolListChanged:     newMethodInfo(clientMethod((*Client).callToolChangedHandler), notification|missingParamsOK),
+	notificationPromptListChanged:   newMethodInfo(clientMethod((*Client).callPromptChangedHandler), notification|missingParamsOK),
+	notificationResourceListChanged: newMethodInfo(clientMethod((*Client).callResourceChangedHandler), notification|missingParamsOK),
+	notificationResourceUpdated:     newMethodInfo(clientMethod((*Client).callResourceUpdatedHandler), notification|missingParamsOK),
+	notificationLoggingMessage:      newMethodInfo(clientMethod((*Client).callLoggingHandler), notification),
+	notificationProgress:            newMethodInfo(sessionMethod((*ClientSession).callProgressNotificationHandler), notification),
 }
 
 func (cs *ClientSession) sendingMethodInfos() map[string]methodInfo {

mcp/server.go

@@ -683,23 +683,27 @@ func (s *Server) AddReceivingMiddleware(middleware ...Middleware[*ServerSession]
 }
 
 // serverMethodInfos maps from the RPC method name to serverMethodInfos.
+//
+// The 'allowMissingParams' values are extracted from the protocol schema.
+// TODO(rfindley): actually load and validate the protocol schema, rather than
+// curating these method flags.
 var serverMethodInfos = map[string]methodInfo{
-	methodComplete:               newMethodInfo(serverMethod((*Server).complete), true),
-	methodInitialize:             newMethodInfo(sessionMethod((*ServerSession).initialize), true),
-	methodPing:                   newMethodInfo(sessionMethod((*ServerSession).ping), true),
-	methodListPrompts:            newMethodInfo(serverMethod((*Server).listPrompts), true),
-	methodGetPrompt:              newMethodInfo(serverMethod((*Server).getPrompt), true),
-	methodListTools:              newMethodInfo(serverMethod((*Server).listTools), true),
-	methodCallTool:               newMethodInfo(serverMethod((*Server).callTool), true),
-	methodListResources:          newMethodInfo(serverMethod((*Server).listResources), true),
-	methodListResourceTemplates:  newMethodInfo(serverMethod((*Server).listResourceTemplates), true),
-	methodReadResource:           newMethodInfo(serverMethod((*Server).readResource), true),
-	methodSetLevel:               newMethodInfo(sessionMethod((*ServerSession).setLevel), true),
-	methodSubscribe:              newMethodInfo(serverMethod((*Server).subscribe), true),
-	methodUnsubscribe:            newMethodInfo(serverMethod((*Server).unsubscribe), true),
-	notificationInitialized:      newMethodInfo(serverMethod((*Server).callInitializedHandler), false),
-	notificationRootsListChanged: newMethodInfo(serverMethod((*Server).callRootsListChangedHandler), false),
-	notificationProgress:         newMethodInfo(sessionMethod((*ServerSession).callProgressNotificationHandler), false),
+	methodComplete:               newMethodInfo(serverMethod((*Server).complete), 0),
+	methodInitialize:             newMethodInfo(sessionMethod((*ServerSession).initialize), 0),
+	methodPing:                   newMethodInfo(sessionMethod((*ServerSession).ping), missingParamsOK),
+	methodListPrompts:            newMethodInfo(serverMethod((*Server).listPrompts), missingParamsOK),
+	methodGetPrompt:              newMethodInfo(serverMethod((*Server).getPrompt), 0),
+	methodListTools:              newMethodInfo(serverMethod((*Server).listTools), missingParamsOK),
+	methodCallTool:               newMethodInfo(serverMethod((*Server).callTool), 0),
+	methodListResources:          newMethodInfo(serverMethod((*Server).listResources), missingParamsOK),
+	methodListResourceTemplates:  newMethodInfo(serverMethod((*Server).listResourceTemplates), missingParamsOK),
+	methodReadResource:           newMethodInfo(serverMethod((*Server).readResource), 0),
+	methodSetLevel:               newMethodInfo(sessionMethod((*ServerSession).setLevel), 0),
+	methodSubscribe:              newMethodInfo(serverMethod((*Server).subscribe), 0),
+	methodUnsubscribe:            newMethodInfo(serverMethod((*Server).unsubscribe), 0),
+	notificationInitialized:      newMethodInfo(serverMethod((*Server).callInitializedHandler), notification|missingParamsOK),
+	notificationRootsListChanged: newMethodInfo(serverMethod((*Server).callRootsListChangedHandler), notification|missingParamsOK),
+	notificationProgress:         newMethodInfo(sessionMethod((*ServerSession).callProgressNotificationHandler), notification),
 }
 
 func (ss *ServerSession) sendingMethodInfos() map[string]methodInfo { return clientMethodInfos }
@@ -744,6 +748,9 @@ func (ss *ServerSession) handle(ctx context.Context, req *jsonrpc.Request) (any,
 }
 
 func (ss *ServerSession) initialize(ctx context.Context, params *InitializeParams) (*InitializeResult, error) {
+	if params == nil {
+		return nil, fmt.Errorf("%w: \"params\" must be be provided", jsonrpc2.ErrInvalidParams)
+	}
 	ss.mu.Lock()
 	ss.initializeParams = params
 	ss.mu.Unlock()

mcp/shared.go

@@ -129,7 +129,7 @@ func handleReceive[S Session](ctx context.Context, session S, req *jsonrpc.Reque
 	}
 	params, err := info.unmarshalParams(req.Params)
 	if err != nil {
-		return nil, fmt.Errorf("handleRequest %q: %w", req.Method, err)
+		return nil, fmt.Errorf("handling '%s': %w", req.Method, err)
 	}
 
 	mh := session.receivingMethodHandler().(MethodHandler[S])
@@ -154,20 +154,28 @@ func checkRequest(req *jsonrpc.Request, infos map[string]methodInfo) (methodInfo
 	if !ok {
 		return methodInfo{}, fmt.Errorf("%w: %q unsupported", jsonrpc2.ErrNotHandled, req.Method)
 	}
-	if info.isRequest && !req.ID.IsValid() {
-		return methodInfo{}, fmt.Errorf("%w: missing ID, %q", jsonrpc2.ErrInvalidRequest, req.Method)
-	}
-	if !info.isRequest && req.ID.IsValid() {
+	if info.flags&notification != 0 && req.ID.IsValid() {
 		return methodInfo{}, fmt.Errorf("%w: unexpected id for %q", jsonrpc2.ErrInvalidRequest, req.Method)
 	}
+	if info.flags&notification == 0 && !req.ID.IsValid() {
+		return methodInfo{}, fmt.Errorf("%w: missing id for %q", jsonrpc2.ErrInvalidRequest, req.Method)
+	}
+	// missingParamsOK is checked here to catch the common case where "params" is
+	// missing entirely.
+	//
+	// However, it's checked again after unmarshalling to catch the rare but
+	// possible case where "params" is JSON null (see https://go.dev/issue/33835).
+	if info.flags&missingParamsOK == 0 && len(req.Params) == 0 {
+		return methodInfo{}, fmt.Errorf("%w: missing required \"params\"", jsonrpc2.ErrInvalidRequest)
+	}
 	return info, nil
 }
 
 // methodInfo is information about sending and receiving a method.
 type methodInfo struct {
-	// isRequest reports whether the method is a JSON-RPC request.
-	// Otherwise, the method is treated as a notification.
-	isRequest bool
+	// flags is a collection of flags controlling how the JSONRPC method is
+	// handled. See individual flag values for documentation.
+	flags methodFlags
 	// Unmarshal params from the wire into a Params struct.
 	// Used on the receive side.
 	unmarshalParams func(json.RawMessage) (Params, error)
@@ -193,20 +201,37 @@ type paramsPtr[T any] interface {
 	Params
 }
 
+type methodFlags int
+
+const (
+	notification    methodFlags = 1 << iota // method is a notification, not request
+	missingParamsOK                         // params may be missing or null
+)
+
 // newMethodInfo creates a methodInfo from a typedMethodHandler.
 //
 // If isRequest is set, the method is treated as a request rather than a
 // notification.
-func newMethodInfo[S Session, P paramsPtr[T], R Result, T any](d typedMethodHandler[S, P, R], isRequest bool) methodInfo {
+func newMethodInfo[S Session, P paramsPtr[T], R Result, T any](d typedMethodHandler[S, P, R], flags methodFlags) methodInfo {
 	return methodInfo{
-		isRequest: isRequest,
+		flags: flags,
 		unmarshalParams: func(m json.RawMessage) (Params, error) {
 			var p P
 			if m != nil {
 				if err := json.Unmarshal(m, &p); err != nil {
 					return nil, fmt.Errorf("unmarshaling %q into a %T: %w", m, p, err)
 				}
 			}
+			// We must check missingParamsOK here, in addition to checkRequest, to
+			// catch the edge cases where "params" is set to JSON null.
+			// See also https://go.dev/issue/33835.
+			//
+			// We need to ensure that p is non-null to guard against crashes, as our
+			// internal code or externally provided handlers may assume that params
+			// is non-null.
+			if flags&missingParamsOK == 0 && p == nil {
+				return nil, fmt.Errorf("%w: missing required \"params\"", jsonrpc2.ErrInvalidRequest)
+			}
 			return orZero[Params](p), nil
 		},
 		handleMethod: MethodHandler[S](func(ctx context.Context, session S, _ string, params Params) (Result, error) {

mcp/sse_test.go

@@ -88,7 +88,7 @@ func TestSSEServer(t *testing.T) {
 					responseContains string
 				}{
 					{"not a method", `{"jsonrpc":"2.0", "method":"notamethod"}`, "not handled"},
-					{"missing ID", `{"jsonrpc":"2.0", "method":"ping"}`, "missing ID"},
+					{"missing ID", `{"jsonrpc":"2.0", "method":"ping"}`, "missing id"},
 				}
 				for _, r := range badRequests {
 					t.Run(r.name, func(t *testing.T) {

mcp/testdata/conformance/server/bad_requests.txtar

@@ -4,10 +4,11 @@ bad requests.
 Fixed bugs:
 - No id in 'initialize' should not panic (#197).
 - No id in 'ping' should not panic (#194).
-- Notifications with IDs should not be treated like requests.
-
-TODO:
 - No params in 'initialize' should not panic (#195).
+- Notifications with IDs should not be treated like requests. (#196)
+- No params in 'logging/setLevel' should not panic.
+- No params in 'completion/complete' should not panic.
+- JSON null params should also not panic in these cases.
 
 -- prompts --
 code_review
@@ -22,6 +23,11 @@ code_review
     "clientInfo": { "name": "ExampleClient", "version": "1.0.0" }
   }
 }
+{
+  "jsonrpc": "2.0",
+  "id": 1,
+  "method": "initialize"
+}
 {
   "jsonrpc": "2.0",
   "id": 2,
@@ -32,10 +38,21 @@ code_review
     "clientInfo": { "name": "ExampleClient", "version": "1.0.0" }
   }
 }
-{"jsonrpc":"2.0", "id": 3, "method":"notifications/initialized"}
+{"jsonrpc":"2.0", "id": 3, "method": "notifications/initialized"}
 {"jsonrpc":"2.0", "method":"ping"}
+{"jsonrpc":"2.0", "id": 4, "method": "logging/setLevel"}
+{"jsonrpc":"2.0", "id": 5, "method": "completion/complete"}
+{"jsonrpc":"2.0", "id": 4, "method": "logging/setLevel", "params": null}
 
 -- server --
+{
+	"jsonrpc": "2.0",
+	"id": 1,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}
 {
 	"jsonrpc": "2.0",
 	"id": 2,
@@ -59,6 +76,30 @@ code_review
 	"id": 3,
 	"error": {
 		"code": -32600,
-		"message": "JSON RPC invalid request: unexpected id for \"notifications/initialized\""
+		"message": "invalid request: unexpected id for \"notifications/initialized\""
+	}
+}
+{
+	"jsonrpc": "2.0",
+	"id": 4,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}
+{
+	"jsonrpc": "2.0",
+	"id": 5,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}
+{
+	"jsonrpc": "2.0",
+	"id": 4,
+	"error": {
+		"code": -32600,
+		"message": "handling 'logging/setLevel': invalid request: missing required \"params\""
 	}
 }

mcp/testdata/conformance/server/prompts.txtar

@@ -1,7 +1,8 @@
 Check behavior of a server with just prompts.
 
 Fixed bugs:
-- empty tools lists should not be returned as 'null'
+- Empty tools lists should not be returned as 'null'.
+- No params in 'prompts/get' should not panic.
 
 -- prompts --
 code_review
@@ -19,6 +20,7 @@ code_review
 }
 { "jsonrpc": "2.0", "id": 2, "method": "tools/list" }
 { "jsonrpc": "2.0", "id": 4, "method": "prompts/list" }
+{ "jsonrpc": "2.0", "id": 5, "method": "prompts/get" }
 -- server --
 {
 	"jsonrpc": "2.0",
@@ -63,3 +65,11 @@ code_review
 		]
 	}
 }
+{
+	"jsonrpc": "2.0",
+	"id": 5,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}

mcp/testdata/conformance/server/resources.txtar

@@ -2,6 +2,9 @@ Check behavior of a server with just resources.
 
 Fixed bugs:
 - A resource result holds a slice of contents, not just one.
+- No params in 'resource/read' should not panic.
+- No params in 'resources/subscribe' should not panic.
+- No params in 'resources/unsubscribe' should not panic.
 
 -- resources --
 info
@@ -39,6 +42,8 @@ info.txt
 	"roots": []
   }
 }
+{ "jsonrpc": "2.0", "id": 4, "method": "resources/read" }
+{ "jsonrpc": "2.0", "id": 5, "method": "resources/subscribe" }
 -- server --
 {
 	"jsonrpc": "2.0",
@@ -107,3 +112,19 @@ info.txt
 		]
 	}
 }
+{
+	"jsonrpc": "2.0",
+	"id": 4,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}
+{
+	"jsonrpc": "2.0",
+	"id": 5,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}

mcp/testdata/conformance/server/tools.txtar

@@ -4,6 +4,7 @@ Fixed bugs:
 - "tools/list" can have missing params
 - "_meta" should not be nil
 - empty resource or prompts should not be returned as 'null'
+- the server should not crash when params are passed to tools/call
 
 -- tools --
 greet
@@ -22,6 +23,7 @@ greet
 { "jsonrpc": "2.0", "id": 2, "method": "tools/list" }
 { "jsonrpc": "2.0", "id": 3, "method": "resources/list" }
 { "jsonrpc": "2.0", "id": 4, "method": "prompts/list" }
+{ "jsonrpc": "2.0", "id": 5, "method": "tools/call" }
 -- server --
 {
 	"jsonrpc": "2.0",
@@ -81,3 +83,11 @@ greet
 		"prompts": []
 	}
 }
+{
+	"jsonrpc": "2.0",
+	"id": 5,
+	"error": {
+		"code": -32600,
+		"message": "invalid request: missing required \"params\""
+	}
+}