From 4e25703d5879fa1acac4fed71ae5f4b8973f170d Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 17 Mar 2021 14:55:05 +0000 Subject: [PATCH] rpc/jsonrpc/server: return an error in WriteRPCResponseHTTP(Error) (bp #6204) (#6230) * rpc/jsonrpc/server: return an error in WriteRPCResponseHTTP(Error) (#6204) instead of panicking Closes #5529 (cherry picked from commit 00b952416836cb568ee904903d30b35a6178bad1) # Conflicts: # CHANGELOG_PENDING.md # rpc/jsonrpc/server/http_json_handler.go # rpc/jsonrpc/server/http_server.go # rpc/jsonrpc/server/http_server_test.go # rpc/jsonrpc/server/http_uri_handler.go * resolve conflicts * fix linting * fix conflict Co-authored-by: Anton Kaliaev Co-authored-by: Marko Baricevic --- CHANGELOG_PENDING.md | 1 + blockchain/v2/reactor_test.go | 5 ++- crypto/hash.go | 2 +- crypto/merkle/proof_value.go | 6 +-- mempool/clist_mempool_test.go | 2 +- p2p/conn/secret_connection.go | 1 - p2p/pex/addrbook.go | 2 +- rpc/jsonrpc/server/http_json_handler.go | 27 ++++++------- rpc/jsonrpc/server/http_server.go | 50 ++++++++++++------------- rpc/jsonrpc/server/http_server_test.go | 10 +++-- rpc/jsonrpc/server/http_uri_handler.go | 30 +++++++++------ statesync/snapshots.go | 6 +-- 12 files changed, 76 insertions(+), 66 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index b72257865..89031d4e5 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -15,6 +15,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - P2P Protocol - Go API + - [rpc/jsonrpc/server] \#6204 Modify `WriteRPCResponseHTTP(Error)` to return an error (@melekes) - Blockchain Protocol diff --git a/blockchain/v2/reactor_test.go b/blockchain/v2/reactor_test.go index 35cedf178..c2792d58b 100644 --- a/blockchain/v2/reactor_test.go +++ b/blockchain/v2/reactor_test.go @@ -59,19 +59,22 @@ func (mp mockPeer) TrySend(byte, []byte) bool { return true } func (mp mockPeer) Set(string, interface{}) {} func (mp mockPeer) Get(string) interface{} { return struct{}{} } -//nolint:unused +// nolint:unused // ignore type mockBlockStore struct { blocks map[int64]*types.Block } +// nolint:unused // ignore func (ml *mockBlockStore) Height() int64 { return int64(len(ml.blocks)) } +// nolint:unused // ignore func (ml *mockBlockStore) LoadBlock(height int64) *types.Block { return ml.blocks[height] } +// nolint:unused // ignore func (ml *mockBlockStore) SaveBlock(block *types.Block, part *types.PartSet, commit *types.Commit) { ml.blocks[block.Height] = block } diff --git a/crypto/hash.go b/crypto/hash.go index dd1b4c1dd..e1d22523f 100644 --- a/crypto/hash.go +++ b/crypto/hash.go @@ -6,6 +6,6 @@ import ( func Sha256(bytes []byte) []byte { hasher := sha256.New() - hasher.Write(bytes) //nolint:errcheck // ignore error + hasher.Write(bytes) return hasher.Sum(nil) } diff --git a/crypto/merkle/proof_value.go b/crypto/merkle/proof_value.go index b613ebe31..ab776216b 100644 --- a/crypto/merkle/proof_value.go +++ b/crypto/merkle/proof_value.go @@ -80,13 +80,13 @@ func (op ValueOp) Run(args [][]byte) ([][]byte, error) { } value := args[0] hasher := tmhash.New() - hasher.Write(value) //nolint: errcheck // does not error + hasher.Write(value) vhash := hasher.Sum(nil) bz := new(bytes.Buffer) // Wrap to hash the KVPair. - encodeByteSlice(bz, op.key) //nolint: errcheck // does not error - encodeByteSlice(bz, vhash) //nolint: errcheck // does not error + encodeByteSlice(bz, op.key) // nolint: errcheck // does not error + encodeByteSlice(bz, vhash) // nolint: errcheck // does not error kvhash := leafHash(bz.Bytes()) if !bytes.Equal(kvhash, op.Proof.LeafHash) { diff --git a/mempool/clist_mempool_test.go b/mempool/clist_mempool_test.go index a40ba69af..6be3f9289 100644 --- a/mempool/clist_mempool_test.go +++ b/mempool/clist_mempool_test.go @@ -662,7 +662,7 @@ func newRemoteApp( } func checksumIt(data []byte) string { h := sha256.New() - h.Write(data) //nolint: errcheck // ignore errcheck + h.Write(data) return fmt.Sprintf("%x", h.Sum(nil)) } diff --git a/p2p/conn/secret_connection.go b/p2p/conn/secret_connection.go index febb975f3..76a93af5f 100644 --- a/p2p/conn/secret_connection.go +++ b/p2p/conn/secret_connection.go @@ -275,7 +275,6 @@ func (sc *SecretConnection) Read(data []byte) (n int, err error) { } // Implements net.Conn -// nolint func (sc *SecretConnection) Close() error { return sc.conn.Close() } func (sc *SecretConnection) LocalAddr() net.Addr { return sc.conn.(net.Conn).LocalAddr() } func (sc *SecretConnection) RemoteAddr() net.Addr { return sc.conn.(net.Conn).RemoteAddr() } diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 6726d15aa..76b21e8dc 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -941,6 +941,6 @@ func (a *addrBook) hash(b []byte) ([]byte, error) { if err != nil { return nil, err } - hasher.Write(b) //nolint:errcheck // ignore error + hasher.Write(b) return hasher.Sum(nil), nil } diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go index 0af2f41a2..28dfcbf8a 100644 --- a/rpc/jsonrpc/server/http_json_handler.go +++ b/rpc/jsonrpc/server/http_json_handler.go @@ -21,14 +21,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, - http.StatusBadRequest, - 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, http.StatusBadRequest, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } return } @@ -48,13 +46,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, - http.StatusInternalServerError, - types.RPCParseError( - fmt.Errorf("error unmarshalling request: %w", err), - ), - ) + res := types.RPCParseError(fmt.Errorf("error unmarshaling request: %w", err)) + if wErr := WriteRPCResponseHTTPError(w, http.StatusInternalServerError, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } return } requests = []types.RPCRequest{request} @@ -108,7 +103,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 6799d3665..bc9785dea 100644 --- a/rpc/jsonrpc/server/http_server.go +++ b/rpc/jsonrpc/server/http_server.go @@ -89,30 +89,32 @@ 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. // -// Panics if it can't Marshal res or write to w. +// source: https://www.jsonrpc.org/historical/json-rpc-over-http.html func WriteRPCResponseHTTPError( w http.ResponseWriter, httpCode int, 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) } 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] @@ -122,13 +124,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 } //----------------------------------------------------------------------------- @@ -166,7 +167,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 @@ -180,15 +183,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, - http.StatusInternalServerError, - 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, http.StatusInternalServerError, 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 60f3ce126..c662e070f 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,9 +161,11 @@ func TestWriteRPCResponseHTTP(t *testing.T) { func TestWriteRPCResponseHTTPError(t *testing.T) { w := httptest.NewRecorder() - WriteRPCResponseHTTPError(w, + err := WriteRPCResponseHTTPError( + w, http.StatusInternalServerError, 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 3e6250183..6609cb837 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/rpc/jsonrpc/server/http_uri_handler.go @@ -25,8 +25,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, http.StatusNotFound, - types.RPCMethodNotFoundError(dummyID)) + res := types.RPCMethodNotFoundError(dummyID) + if wErr := WriteRPCResponseHTTPError(w, http.StatusNotFound, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } } } @@ -39,14 +41,12 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit fnArgs, err := httpParamsToArgs(rpcFunc, r) if err != nil { - WriteRPCResponseHTTPError( - w, - http.StatusInternalServerError, - 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, http.StatusInternalServerError, res); wErr != nil { + logger.Error("failed to write response", "res", res, "err", wErr) + } return } args = append(args, fnArgs...) @@ -56,11 +56,17 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit logger.Debug("HTTPRestRPC", "method", r.URL.Path, "args", args, "returns", returns) result, err := unreflectResult(returns) if err != nil { - WriteRPCResponseHTTPError(w, http.StatusInternalServerError, - types.RPCInternalError(dummyID, err)) + if err := WriteRPCResponseHTTPError(w, http.StatusInternalServerError, + types.RPCInternalError(dummyID, err)); err != nil { + logger.Error("failed to write response", "res", result, "err", err) + return + } + return + } + if err := WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result)); err != nil { + logger.Error("failed to write response", "res", result, "err", err) return } - WriteRPCResponseHTTP(w, types.NewRPCSuccessResponse(dummyID, result)) } } diff --git a/statesync/snapshots.go b/statesync/snapshots.go index 3eca5c0be..a9ebdc9eb 100644 --- a/statesync/snapshots.go +++ b/statesync/snapshots.go @@ -32,9 +32,9 @@ type snapshot struct { func (s *snapshot) Key() snapshotKey { // Hash.Write() never returns an error. hasher := sha256.New() - hasher.Write([]byte(fmt.Sprintf("%v:%v:%v", s.Height, s.Format, s.Chunks))) //nolint:errcheck // ignore error - hasher.Write(s.Hash) //nolint:errcheck // ignore error - hasher.Write(s.Metadata) //nolint:errcheck // ignore error + hasher.Write([]byte(fmt.Sprintf("%v:%v:%v", s.Height, s.Format, s.Chunks))) + hasher.Write(s.Hash) + hasher.Write(s.Metadata) var key snapshotKey copy(key[:], hasher.Sum(nil)) return key