From 59556ab030345b5a2d4f7de16850a71e56c172a7 Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Sat, 14 Oct 2017 19:58:47 -0600 Subject: [PATCH 1/4] rpc/lib/server: add handlers tests Follow up of PR https://github.com/tendermint/tendermint/pull/724 For https://github.com/tendermint/tendermint/issues/708 Reported initially in #708, this bug was reconfirmed by the fuzzer. This fix ensures that: * if the user doesn't pass in `"id"` that we send them back a message in an error telling them to send `"id"`. Previously we let the handler return a 200 with nothing. * passing in nil `params` doesn't crash * not passing in `params` doesn't crash * passing in non-JSON parseable data to `params` doesn't crash --- rpc/lib/server/handlers.go | 2 + rpc/lib/server/handlers_test.go | 81 +++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 rpc/lib/server/handlers_test.go diff --git a/rpc/lib/server/handlers.go b/rpc/lib/server/handlers.go index 3a3c48f02..0b379b9ab 100644 --- a/rpc/lib/server/handlers.go +++ b/rpc/lib/server/handlers.go @@ -118,6 +118,8 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han // The Server MUST NOT reply to a Notification, including those that are within a batch request. if request.ID == "" { logger.Debug("HTTPJSONRPC received a notification, skipping... (please send a non-empty ID if you want to call a method)") + // Not sending back a response here because according the JSONRPC + // specification Section 4.1, we SHOULD NOT one back when "id" == "". return } if len(r.URL.Path) > 1 { diff --git a/rpc/lib/server/handlers_test.go b/rpc/lib/server/handlers_test.go new file mode 100644 index 000000000..c816ede72 --- /dev/null +++ b/rpc/lib/server/handlers_test.go @@ -0,0 +1,81 @@ +package rpcserver_test + +import ( + "bytes" + "encoding/json" + "io/ioutil" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + + rs "github.com/tendermint/tendermint/rpc/lib/server" + types "github.com/tendermint/tendermint/rpc/lib/types" + "github.com/tendermint/tmlibs/log" +) + +// Ensure that nefarious/unintended inputs to `params` +// do not crash our RPC handlers. +// See Issue https://github.com/tendermint/tendermint/issues/708. +func TestRPCParams(t *testing.T) { + funcMap := map[string]*rs.RPCFunc{ + "c": rs.NewRPCFunc(func(s string, i int) (string, error) { return "foo", nil }, "s,i"), + } + mux := http.NewServeMux() + buf := new(bytes.Buffer) + logger := log.NewTMLogger(buf) + rs.RegisterRPCFuncs(mux, funcMap, logger) + + tests := []struct { + payload string + wantErr string + notification bool + }{ + {`{"jsonrpc": "2.0"}`, "", true}, // The server SHOULD NOT respond to a notification according to JSONRPC Section 4.1. + {`{"jsonrpc": "2.0", "id": "0"}`, "Method not found", false}, + {`{"jsonrpc": "2.0", "method": "y", "id": "0"}`, "Method not found", false}, + {`{"jsonrpc": "2.0", "method": "c", "id": "0", "params": null}`, "", false}, + {`{"method": "c", "id": "0", "params": {}}`, "", false}, + {`{"method": "c", "id": "0", "params": a}`, "invalid character", false}, + {`{"method": "c", "id": "0", "params": ["a", 10]}`, "", false}, + {`{"method": "c", "id": "0", "params": ["a"]}`, "got 1", false}, + {`{"method": "c", "id": "0", "params": ["a", "b"]}`, "of type int", false}, + {`{"method": "c", "id": "0", "params": [1, 1]}`, "of type string", false}, + } + + statusOK := func(code int) bool { return code >= 200 && code <= 299 } + + for i, tt := range tests { + req, _ := http.NewRequest("POST", "http://localhost/", strings.NewReader(tt.payload)) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + res := rec.Result() + // Always expecting back a JSONRPCResponse + assert.True(t, statusOK(res.StatusCode), "#%d: should always return 2XX", i) + blob, err := ioutil.ReadAll(res.Body) + if err != nil { + t.Errorf("#%d: err reading body: %v", i, err) + continue + } + + if tt.notification { + assert.Equal(t, len(blob), 0, "#%d: a notification SHOULD NOT be responded to by the server", i) + continue + } + + recv := new(types.RPCResponse) + assert.Nil(t, json.Unmarshal(blob, recv), "#%d: expecting successful parsing of an RPCResponse:\nblob: %s", i, blob) + assert.NotEqual(t, recv, new(types.RPCResponse), "#%d: not expecting a blank RPCResponse", i) + + if tt.wantErr == "" { + assert.Nil(t, recv.Error, "#%d: not expecting an error", i) + } else { + assert.False(t, statusOK(recv.Error.Code), "#%d: not expecting a 2XX success code", i) + // The wanted error is either in the message or the data + assert.Contains(t, recv.Error.Message+recv.Error.Data, tt.wantErr, "#%d: expected substring", i) + continue + } + } +} From e7fab7d4bf1fb544e519408af07c75805898c1b2 Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Fri, 20 Oct 2017 21:24:21 -0700 Subject: [PATCH 2/4] rpc/lib/server: update with @melekes and @ebuchman feedback --- rpc/lib/server/handlers.go | 2 -- rpc/lib/server/handlers_test.go | 1 - 2 files changed, 3 deletions(-) diff --git a/rpc/lib/server/handlers.go b/rpc/lib/server/handlers.go index 0b379b9ab..3a3c48f02 100644 --- a/rpc/lib/server/handlers.go +++ b/rpc/lib/server/handlers.go @@ -118,8 +118,6 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger log.Logger) http.Han // The Server MUST NOT reply to a Notification, including those that are within a batch request. if request.ID == "" { logger.Debug("HTTPJSONRPC received a notification, skipping... (please send a non-empty ID if you want to call a method)") - // Not sending back a response here because according the JSONRPC - // specification Section 4.1, we SHOULD NOT one back when "id" == "". return } if len(r.URL.Path) > 1 { diff --git a/rpc/lib/server/handlers_test.go b/rpc/lib/server/handlers_test.go index c816ede72..16467558b 100644 --- a/rpc/lib/server/handlers_test.go +++ b/rpc/lib/server/handlers_test.go @@ -75,7 +75,6 @@ func TestRPCParams(t *testing.T) { assert.False(t, statusOK(recv.Error.Code), "#%d: not expecting a 2XX success code", i) // The wanted error is either in the message or the data assert.Contains(t, recv.Error.Message+recv.Error.Data, tt.wantErr, "#%d: expected substring", i) - continue } } } From a8b77359dfb9c630291279ecb03c198d26e6012e Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Sat, 28 Oct 2017 15:24:35 -0700 Subject: [PATCH 3/4] rpc/lib/server: separate out Notifications test Addressing feedback from @ebuchman --- rpc/lib/server/handlers_test.go | 66 ++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/rpc/lib/server/handlers_test.go b/rpc/lib/server/handlers_test.go index 16467558b..2260f73d3 100644 --- a/rpc/lib/server/handlers_test.go +++ b/rpc/lib/server/handlers_test.go @@ -10,16 +10,14 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" rs "github.com/tendermint/tendermint/rpc/lib/server" types "github.com/tendermint/tendermint/rpc/lib/types" "github.com/tendermint/tmlibs/log" ) -// Ensure that nefarious/unintended inputs to `params` -// do not crash our RPC handlers. -// See Issue https://github.com/tendermint/tendermint/issues/708. -func TestRPCParams(t *testing.T) { +func testMux() *http.ServeMux { funcMap := map[string]*rs.RPCFunc{ "c": rs.NewRPCFunc(func(s string, i int) (string, error) { return "foo", nil }, "s,i"), } @@ -28,24 +26,30 @@ func TestRPCParams(t *testing.T) { logger := log.NewTMLogger(buf) rs.RegisterRPCFuncs(mux, funcMap, logger) - tests := []struct { - payload string - wantErr string - notification bool - }{ - {`{"jsonrpc": "2.0"}`, "", true}, // The server SHOULD NOT respond to a notification according to JSONRPC Section 4.1. - {`{"jsonrpc": "2.0", "id": "0"}`, "Method not found", false}, - {`{"jsonrpc": "2.0", "method": "y", "id": "0"}`, "Method not found", false}, - {`{"jsonrpc": "2.0", "method": "c", "id": "0", "params": null}`, "", false}, - {`{"method": "c", "id": "0", "params": {}}`, "", false}, - {`{"method": "c", "id": "0", "params": a}`, "invalid character", false}, - {`{"method": "c", "id": "0", "params": ["a", 10]}`, "", false}, - {`{"method": "c", "id": "0", "params": ["a"]}`, "got 1", false}, - {`{"method": "c", "id": "0", "params": ["a", "b"]}`, "of type int", false}, - {`{"method": "c", "id": "0", "params": [1, 1]}`, "of type string", false}, - } + return mux +} - statusOK := func(code int) bool { return code >= 200 && code <= 299 } +func statusOK(code int) bool { return code >= 200 && code <= 299 } + +// Ensure that nefarious/unintended inputs to `params` +// do not crash our RPC handlers. +// See Issue https://github.com/tendermint/tendermint/issues/708. +func TestRPCParams(t *testing.T) { + mux := testMux() + tests := []struct { + payload string + wantErr string + }{ + {`{"jsonrpc": "2.0", "id": "0"}`, "Method not found"}, + {`{"jsonrpc": "2.0", "method": "y", "id": "0"}`, "Method not found"}, + {`{"jsonrpc": "2.0", "method": "c", "id": "0", "params": null}`, ""}, + {`{"method": "c", "id": "0", "params": {}}`, ""}, + {`{"method": "c", "id": "0", "params": a}`, "invalid character"}, + {`{"method": "c", "id": "0", "params": ["a", 10]}`, ""}, + {`{"method": "c", "id": "0", "params": ["a"]}`, "got 1"}, + {`{"method": "c", "id": "0", "params": ["a", "b"]}`, "of type int"}, + {`{"method": "c", "id": "0", "params": [1, 1]}`, "of type string"}, + } for i, tt := range tests { req, _ := http.NewRequest("POST", "http://localhost/", strings.NewReader(tt.payload)) @@ -60,11 +64,6 @@ func TestRPCParams(t *testing.T) { continue } - if tt.notification { - assert.Equal(t, len(blob), 0, "#%d: a notification SHOULD NOT be responded to by the server", i) - continue - } - recv := new(types.RPCResponse) assert.Nil(t, json.Unmarshal(blob, recv), "#%d: expecting successful parsing of an RPCResponse:\nblob: %s", i, blob) assert.NotEqual(t, recv, new(types.RPCResponse), "#%d: not expecting a blank RPCResponse", i) @@ -78,3 +77,18 @@ func TestRPCParams(t *testing.T) { } } } + +func TestRPCNotification(t *testing.T) { + mux := testMux() + body := strings.NewReader(`{"jsonrpc": "2.0"}`) + req, _ := http.NewRequest("POST", "http://localhost/", body) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + res := rec.Result() + + // Always expecting back a JSONRPCResponse + require.True(t, statusOK(res.StatusCode), "should always return 2XX") + blob, err := ioutil.ReadAll(res.Body) + require.Nil(t, err, "reading from the body should not give back an error") + require.Equal(t, len(blob), 0, "a notification SHOULD NOT be responded to by the server") +} From f7f4ba5e90a76cb17bb251e6bcd68c19ac841dc7 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 31 Oct 2017 15:41:25 -0400 Subject: [PATCH 4/4] rpc/lib/server: minor changes to test --- rpc/lib/server/handlers_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/rpc/lib/server/handlers_test.go b/rpc/lib/server/handlers_test.go index 2260f73d3..664bbd917 100644 --- a/rpc/lib/server/handlers_test.go +++ b/rpc/lib/server/handlers_test.go @@ -40,15 +40,18 @@ func TestRPCParams(t *testing.T) { payload string wantErr string }{ + // bad {`{"jsonrpc": "2.0", "id": "0"}`, "Method not found"}, {`{"jsonrpc": "2.0", "method": "y", "id": "0"}`, "Method not found"}, - {`{"jsonrpc": "2.0", "method": "c", "id": "0", "params": null}`, ""}, - {`{"method": "c", "id": "0", "params": {}}`, ""}, {`{"method": "c", "id": "0", "params": a}`, "invalid character"}, - {`{"method": "c", "id": "0", "params": ["a", 10]}`, ""}, {`{"method": "c", "id": "0", "params": ["a"]}`, "got 1"}, {`{"method": "c", "id": "0", "params": ["a", "b"]}`, "of type int"}, {`{"method": "c", "id": "0", "params": [1, 1]}`, "of type string"}, + + // good + {`{"jsonrpc": "2.0", "method": "c", "id": "0", "params": null}`, ""}, + {`{"method": "c", "id": "0", "params": {}}`, ""}, + {`{"method": "c", "id": "0", "params": ["a", 10]}`, ""}, } for i, tt := range tests { @@ -71,7 +74,7 @@ func TestRPCParams(t *testing.T) { if tt.wantErr == "" { assert.Nil(t, recv.Error, "#%d: not expecting an error", i) } else { - assert.False(t, statusOK(recv.Error.Code), "#%d: not expecting a 2XX success code", i) + assert.True(t, recv.Error.Code < 0, "#%d: not expecting a positive JSONRPC code", i) // The wanted error is either in the message or the data assert.Contains(t, recv.Error.Message+recv.Error.Data, tt.wantErr, "#%d: expected substring", i) }