diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 0f688ff82..3d7fe5d6e 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -42,6 +42,7 @@ Friendly reminder: We have a [bug bounty program](https://hackerone.com/tendermi - [rpc/client/http] \#6176 Remove `endpoint` arg from `New`, `NewWithTimeout` and `NewWithClient` (@melekes) - [rpc/client/http] \#6176 Unexpose `WSEvents` (@melekes) - [rpc/jsonrpc/client/ws_client] \#6176 `NewWS` no longer accepts options (use `NewWSWithOptions` and `OnReconnect` funcs to configure the client) (@melekes) + - [rpc/jsonrpc/server] \#6204 Modify `WriteRPCResponseHTTP(Error)` to return an error (@melekes) - Blockchain Protocol diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go index 1f79dcd9c..17cba3828 100644 --- a/rpc/jsonrpc/server/http_json_handler.go +++ b/rpc/jsonrpc/server/http_json_handler.go @@ -23,13 +23,12 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han return func(w http.ResponseWriter, r *http.Request) { b, err := ioutil.ReadAll(r.Body) if err != nil { - WriteRPCResponseHTTPError( - w, - types.RPCInvalidRequestError( - nil, - fmt.Errorf("error reading request body: %w", err), - ), + res := types.RPCInvalidRequestError(nil, + fmt.Errorf("error reading request body: %w", err), ) + if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } return } @@ -49,12 +48,10 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han // next, try to unmarshal as a single request var request types.RPCRequest if err := json.Unmarshal(b, &request); err != nil { - WriteRPCResponseHTTPError( - w, - types.RPCParseError( - fmt.Errorf("error unmarshaling request: %w", err), - ), - ) + res := types.RPCParseError(fmt.Errorf("error unmarshaling request: %w", err)) + if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } return } requests = []types.RPCRequest{request} @@ -125,7 +122,9 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han } if len(responses) > 0 { - WriteRPCResponseHTTP(w, responses...) + if wErr := WriteRPCResponseHTTP(w, responses...); wErr != nil { + logger.Error("failed to write responses", "res", responses, "err", wErr) + } } } } diff --git a/rpc/jsonrpc/server/http_server.go b/rpc/jsonrpc/server/http_server.go index 52d46740b..86dbed4fa 100644 --- a/rpc/jsonrpc/server/http_server.go +++ b/rpc/jsonrpc/server/http_server.go @@ -89,7 +89,8 @@ func ServeTLS( return err } -// WriteRPCResponseHTTPError marshals res as JSON and writes it to w. +// WriteRPCResponseHTTPError marshals res as JSON (with indent) and writes it +// to w. // // Maps JSON RPC error codes to HTTP Status codes as follows: // @@ -102,19 +103,17 @@ func ServeTLS( // 500 -32099..-32000 Server error. // // source: https://www.jsonrpc.org/historical/json-rpc-over-http.html -// -// Panics if it can't Marshal res or write to w. func WriteRPCResponseHTTPError( w http.ResponseWriter, res types.RPCResponse, -) { +) error { if res.Error == nil { panic("tried to write http error response without RPC error") } jsonBytes, err := json.MarshalIndent(res, "", " ") if err != nil { - panic(err) + return fmt.Errorf("json marshal: %w", err) } var httpCode int @@ -129,15 +128,12 @@ func WriteRPCResponseHTTPError( w.Header().Set("Content-Type", "application/json") w.WriteHeader(httpCode) - if _, err := w.Write(jsonBytes); err != nil { - panic(err) - } + _, err = w.Write(jsonBytes) + return err } -// WriteRPCResponseHTTP marshals res as JSON and writes it to w. -// -// Panics if it can't Marshal res or write to w. -func WriteRPCResponseHTTP(w http.ResponseWriter, res ...types.RPCResponse) { +// WriteRPCResponseHTTP marshals res as JSON (with indent) and writes it to w. +func WriteRPCResponseHTTP(w http.ResponseWriter, res ...types.RPCResponse) error { var v interface{} if len(res) == 1 { v = res[0] @@ -147,13 +143,12 @@ func WriteRPCResponseHTTP(w http.ResponseWriter, res ...types.RPCResponse) { jsonBytes, err := json.MarshalIndent(v, "", " ") if err != nil { - panic(err) + return fmt.Errorf("json marshal: %w", err) } w.Header().Set("Content-Type", "application/json") w.WriteHeader(200) - if _, err := w.Write(jsonBytes); err != nil { - panic(err) - } + _, err = w.Write(jsonBytes) + return err } //----------------------------------------------------------------------------- @@ -191,7 +186,9 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler // If RPCResponse if res, ok := e.(types.RPCResponse); ok { - WriteRPCResponseHTTP(rww, res) + if wErr := WriteRPCResponseHTTP(rww, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } } else { // Panics can contain anything, attempt to normalize it as an error. var err error @@ -205,14 +202,12 @@ func RecoverAndLogHandler(handler http.Handler, logger log.Logger) http.Handler default: } - logger.Error( - "Panic in RPC HTTP handler", "err", e, "stack", - string(debug.Stack()), - ) - WriteRPCResponseHTTPError( - rww, - types.RPCInternalError(types.JSONRPCIntID(-1), err), - ) + logger.Error("panic in RPC HTTP handler", "err", e, "stack", string(debug.Stack())) + + res := types.RPCInternalError(types.JSONRPCIntID(-1), err) + if wErr := WriteRPCResponseHTTPError(rww, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } } } diff --git a/rpc/jsonrpc/server/http_server_test.go b/rpc/jsonrpc/server/http_server_test.go index 98e1adf25..f4da2f971 100644 --- a/rpc/jsonrpc/server/http_server_test.go +++ b/rpc/jsonrpc/server/http_server_test.go @@ -112,7 +112,8 @@ func TestWriteRPCResponseHTTP(t *testing.T) { // one argument w := httptest.NewRecorder() - WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(id, &sampleResult{"hello"})) + err := WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(id, &sampleResult{"hello"})) + require.NoError(t, err) resp := w.Result() body, err := ioutil.ReadAll(resp.Body) _ = resp.Body.Close() @@ -129,9 +130,10 @@ func TestWriteRPCResponseHTTP(t *testing.T) { // multiple arguments w = httptest.NewRecorder() - WriteRPCResponseHTTP(w, + err = WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(id, &sampleResult{"hello"}), types.NewRPCSuccessResponse(id, &sampleResult{"world"})) + require.NoError(t, err) resp = w.Result() body, err = ioutil.ReadAll(resp.Body) _ = resp.Body.Close() @@ -159,7 +161,8 @@ func TestWriteRPCResponseHTTP(t *testing.T) { func TestWriteRPCResponseHTTPError(t *testing.T) { w := httptest.NewRecorder() - WriteRPCResponseHTTPError(w, types.RPCInternalError(types.JSONRPCIntID(-1), errors.New("foo"))) + err := WriteRPCResponseHTTPError(w, types.RPCInternalError(types.JSONRPCIntID(-1), errors.New("foo"))) + require.NoError(t, err) resp := w.Result() body, err := ioutil.ReadAll(resp.Body) _ = resp.Body.Close() diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go index d5c2a44d8..cd03383f7 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/rpc/jsonrpc/server/http_uri_handler.go @@ -27,7 +27,10 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit // Exception for websocket endpoints if rpcFunc.ws { return func(w http.ResponseWriter, r *http.Request) { - WriteRPCResponseHTTPError(w, types.RPCMethodNotFoundError(dummyID)) + res := types.RPCMethodNotFoundError(dummyID) + if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } } } @@ -40,13 +43,12 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit fnArgs, err := httpParamsToArgs(rpcFunc, r) if err != nil { - WriteRPCResponseHTTPError( - w, - types.RPCInvalidParamsError( - dummyID, - fmt.Errorf("error converting http params to arguments: %w", err), - ), + res := types.RPCInvalidParamsError(dummyID, + fmt.Errorf("error converting http params to arguments: %w", err), ) + if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } return } args = append(args, fnArgs...) @@ -58,21 +60,33 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit switch e := err.(type) { // if no error then return a success response case nil: - WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result)) + res := types.NewRPCSuccessResponse(dummyID, result) + if wErr := WriteRPCResponseHTTP(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } // if this already of type RPC error then forward that error. case *types.RPCError: - WriteRPCResponseHTTPError(w, types.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data)) + res := types.NewRPCErrorResponse(dummyID, e.Code, e.Message, e.Data) + if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } default: // we need to unwrap the error and parse it accordingly + var res types.RPCResponse switch errors.Unwrap(err) { - case ctypes.ErrZeroOrNegativeHeight, ctypes.ErrZeroOrNegativePerPage, - ctypes.ErrPageOutOfRange, ctypes.ErrInvalidRequest: - WriteRPCResponseHTTPError(w, types.RPCInvalidRequestError(dummyID, err)) - + case ctypes.ErrZeroOrNegativeHeight, + ctypes.ErrZeroOrNegativePerPage, + ctypes.ErrPageOutOfRange, + ctypes.ErrInvalidRequest: + res = types.RPCInvalidRequestError(dummyID, err) default: // ctypes.ErrHeightNotAvailable, ctypes.ErrHeightExceedsChainHead: - WriteRPCResponseHTTPError(w, types.RPCInternalError(dummyID, err)) + res = types.RPCInternalError(dummyID, err) + } + + if wErr := WriteRPCResponseHTTPError(w, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) } }