diff --git a/jsonrpc/server.go b/jsonrpc/server.go index 0fd2eeb3e6..bdee56b73e 100644 --- a/jsonrpc/server.go +++ b/jsonrpc/server.go @@ -11,6 +11,7 @@ import ( "io" "net/http" "reflect" + "runtime/debug" "strings" "sync" "time" @@ -568,18 +569,41 @@ func isNilOrEmpty(i any) (bool, error) { } } -// TODO: add recover() to catch panics from handlers/validators and return a JSON-RPC internal error -// instead of crashing the HTTP connection -func (s *Server) handleRequest(ctx context.Context, req *Request) (*response, http.Header, error) { +func (s *Server) handleRequest( + ctx context.Context, + req *Request, +) (res *response, header http.Header, resErr error) { s.logger.Trace("Received request", zap.Object("req", req)) - header := http.Header{} + header = http.Header{} + + defer func() { + if r := recover(); r != nil { + s.logger.Error("Recovered from panic while handling RPC request", + zap.String("method", req.Method), + zap.Any("panic", r), + zap.ByteString("stack", debug.Stack()), + ) + resErr = nil + if req.ID == nil { // notification: the spec forbids a response either way + res = nil + return + } + res = &response{ + Version: "2.0", + ID: req.ID, + Error: Err(InternalError, nil), + } + s.listener.OnRequestFailed(req.Method, res.Error) + } + }() + if err := req.isSane(); err != nil { s.logger.Trace("Request sanity check failed", zap.Error(err)) return nil, header, err } - res := &response{ + res = &response{ Version: "2.0", ID: req.ID, } diff --git a/jsonrpc/server_test.go b/jsonrpc/server_test.go index 4c7167fe2b..8d40d065e7 100644 --- a/jsonrpc/server_test.go +++ b/jsonrpc/server_test.go @@ -93,6 +93,9 @@ func TestHandle(t *testing.T) { type validationStruct struct { A int `validate:"min=1"` } + type panickingValidationStruct struct { + A int `validate:"panics"` + } methods := []jsonrpc.Method{ { Name: "method", @@ -155,6 +158,20 @@ func TestHandle(t *testing.T) { return v["expectedkey"].A, nil }, }, + { + Name: "panics", + Params: []jsonrpc.Parameter{}, + Handler: func() (int, *jsonrpc.Error) { + panic("handler panic") + }, + }, + { + Name: "validationPanics", + Params: []jsonrpc.Parameter{{Name: "param"}}, + Handler: func(v panickingValidationStruct) (int, *jsonrpc.Error) { + return v.A, nil + }, + }, { Name: "acceptsContext", Handler: func(ctx context.Context) (int, *jsonrpc.Error) { @@ -198,9 +215,15 @@ func TestHandle(t *testing.T) { }, } + v := validator.New() + // register a custom validation tag to simulate a validator panic from validateParam + require.NoError(t, v.RegisterValidation("panics", func(fl validator.FieldLevel) bool { + panic("validator panic") + })) + listener := CountingEventListener{} server := jsonrpc.NewServer(1, log.NewNopZapLogger()). - WithValidator(validator.New()). + WithValidator(v). WithListener(&listener) require.NoError(t, server.RegisterMethods(methods...)) @@ -544,6 +567,33 @@ func TestHandle(t *testing.T) { req: `{"jsonrpc": "2.0", "method": "multipleOptionalParams", "params": {"param1": 1, "param2": [2, 3], "junk": "junk"}, "id": 1}`, res: `{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid Params","data":"unexpected params: junk"},"id":1}`, }, + + "handler panics": { + req: `{"jsonrpc": "2.0", "method": "panics", "params": {}, "id": 1}`, + res: `{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"},"id":1}`, + checkFailedEvent: true, + }, + + "handler panics as notification": { + req: `{"jsonrpc": "2.0", "method": "panics", "params": {}}`, + res: ``, + }, + + "validator panics": { + req: `{"jsonrpc": "2.0", "method": "validationPanics", "params": [{"A": 1}], "id": 1}`, + res: `{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"},"id":1}`, + checkFailedEvent: true, + }, + + "handler panic mixed with successes in a batch": { + req: `[{"jsonrpc" : "2.0", "method" : "method", + "params" : { "num" : 5 } , "id" : 5}, + {"jsonrpc" : "2.0", "method" : "panics", + "params" : {} , "id" : 8}, + {"jsonrpc" : "2.0", "method" : "method", + "params" : { "num" : 44 } , "id" : 6}]`, + res: `[{"jsonrpc":"2.0","result":{"doubled":10},"id":5},{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"},"id":8},{"jsonrpc":"2.0","result":{"doubled":88},"id":6}]`, + }, } for desc, test := range tests {