diff --git a/internal/jsonrpc2/wire.go b/internal/jsonrpc2/wire.go index 309b800..b143dcd 100644 --- a/internal/jsonrpc2/wire.go +++ b/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" diff --git a/mcp/client.go b/mcp/client.go index 2cd81e0..9e3f393 100644 --- a/mcp/client.go +++ b/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 { diff --git a/mcp/server.go b/mcp/server.go index 220c28d..08effd7 100644 --- a/mcp/server.go +++ b/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() diff --git a/mcp/shared.go b/mcp/shared.go index 46720ee..54038f5 100644 --- a/mcp/shared.go +++ b/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¬ification != 0 && req.ID.IsValid() { return methodInfo{}, fmt.Errorf("%w: unexpected id for %q", jsonrpc2.ErrInvalidRequest, req.Method) } + if info.flags¬ification == 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,13 +201,20 @@ 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 { @@ -207,6 +222,16 @@ func newMethodInfo[S Session, P paramsPtr[T], R Result, T any](d typedMethodHand 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) { diff --git a/mcp/sse_test.go b/mcp/sse_test.go index d486732..35fdbdb 100644 --- a/mcp/sse_test.go +++ b/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) { diff --git a/mcp/testdata/conformance/server/bad_requests.txtar b/mcp/testdata/conformance/server/bad_requests.txtar index d2f278b..a8767ad 100644 --- a/mcp/testdata/conformance/server/bad_requests.txtar +++ b/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\"" } } diff --git a/mcp/testdata/conformance/server/prompts.txtar b/mcp/testdata/conformance/server/prompts.txtar index 078e091..8eb678c 100644 --- a/mcp/testdata/conformance/server/prompts.txtar +++ b/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\"" + } +} diff --git a/mcp/testdata/conformance/server/resources.txtar b/mcp/testdata/conformance/server/resources.txtar index 5bb5515..fe9a1b7 100644 --- a/mcp/testdata/conformance/server/resources.txtar +++ b/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\"" + } +} diff --git a/mcp/testdata/conformance/server/tools.txtar b/mcp/testdata/conformance/server/tools.txtar index 07ad942..7525ac1 100644 --- a/mcp/testdata/conformance/server/tools.txtar +++ b/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\"" + } +}