rpc/jsonrpc/server: return an error in WriteRPCResponseHTTP(Error) (#6204)

instead of panicking
Closes #5529

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
 

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)
+			}
 		}
 	}
 }

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)
+					}
 				}
 			}
 

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()

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)
 			}
 		}