mcp: handle bad or missing params (#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 #195

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,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) {