From 1ff69361e811a7ba3b7d4dc940a437c4ac322c4e Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Fri, 14 Jan 2022 07:53:53 -0800 Subject: [PATCH] rpc: remove dependency of URL (GET) requests on tmjson (#7590) The parameters for RPC GET requests are parsed from query arguments in the request URL. Rework this code to remove the need for tmjson. The structure of a call still requires reflection, and still works the same way as before, but the code structure has been simplified and cleaned up a bit. Points of note: - Consolidate handling of pointer types, so we only need to dereference once. - Reduce the number of allocations of reflective types. - Report errors for unsupported types rather than returning untyped nil. Update the tests as well. There was one test case that checked for an error on a behaviour the OpenAPI docs explicitly demonstrates as supported, so I fixed that test case, and also added some new ones for cases that weren't checked. Related: * Update e2e base Go image to 1.17 (to match config). --- rpc/jsonrpc/server/http_uri_handler.go | 243 ++++++++++++------------- rpc/jsonrpc/server/parse_test.go | 19 +- test/e2e/docker/Dockerfile | 2 +- 3 files changed, 131 insertions(+), 133 deletions(-) diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go index 808da16e2..0728b1f08 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/rpc/jsonrpc/server/http_uri_handler.go @@ -1,15 +1,16 @@ package server import ( + "context" "encoding/hex" + "encoding/json" "errors" "fmt" "net/http" "reflect" - "regexp" + "strconv" "strings" - tmjson "github.com/tendermint/tendermint/libs/json" "github.com/tendermint/tendermint/libs/log" "github.com/tendermint/tendermint/rpc/coretypes" rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types" @@ -17,8 +18,6 @@ import ( // HTTP + URI handler -var reInt = regexp.MustCompile(`^-?[0-9]+$`) - // convert from a function name to the http handler func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWriter, *http.Request) { // Always return -1 as there's no ID here. @@ -35,22 +34,21 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit } // All other endpoints - return func(w http.ResponseWriter, r *http.Request) { - ctx := rpctypes.WithCallInfo(r.Context(), &rpctypes.CallInfo{HTTPRequest: r}) - args := []reflect.Value{reflect.ValueOf(ctx)} - - fnArgs, err := httpParamsToArgs(rpcFunc, r) + return func(w http.ResponseWriter, req *http.Request) { + ctx := rpctypes.WithCallInfo(req.Context(), &rpctypes.CallInfo{ + HTTPRequest: req, + }) + args, err := parseURLParams(ctx, rpcFunc, req) if err != nil { - writeHTTPResponse(w, logger, rpctypes.RPCInvalidParamsError( - dummyID, fmt.Errorf("error converting http params to arguments: %w", err))) + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprintln(w, err.Error()) return } - args = append(args, fnArgs...) - - returns := rpcFunc.f.Call(args) + outs := rpcFunc.f.Call(args) - logger.Debug("HTTPRestRPC", "method", r.URL.Path, "args", args, "returns", returns) - result, err := unreflectResult(returns) + logger.Debug("HTTPRestRPC", "method", req.URL.Path, "args", args, "returns", outs) + result, err := unreflectResult(outs) switch e := err.(type) { // if no error then return a success response case nil: @@ -74,142 +72,135 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit } } -// Covert an http query to a list of properly typed values. -// To be properly decoded the arg must be a concrete type from tendermint (if its an interface). -func httpParamsToArgs(rpcFunc *RPCFunc, r *http.Request) ([]reflect.Value, error) { - // skip types.Context - const argsOffset = 1 - - values := make([]reflect.Value, len(rpcFunc.argNames)) - - for i, name := range rpcFunc.argNames { - argType := rpcFunc.args[i+argsOffset] - - values[i] = reflect.Zero(argType) // set default for that type +func parseURLParams(ctx context.Context, rf *RPCFunc, req *http.Request) ([]reflect.Value, error) { + if err := req.ParseForm(); err != nil { + return nil, fmt.Errorf("invalid HTTP request: %w", err) + } + getArg := func(name string) (string, bool) { + if req.Form.Has(name) { + return req.Form.Get(name), true + } + return "", false + } - arg := getParam(r, name) - // log.Notice("param to arg", "argType", argType, "name", name, "arg", arg) + vals := make([]reflect.Value, len(rf.argNames)+1) + vals[0] = reflect.ValueOf(ctx) + for i, name := range rf.argNames { + atype := rf.args[i+1] - if arg == "" { + text, ok := getArg(name) + if !ok { + vals[i+1] = reflect.Zero(atype) continue } - v, ok, err := nonJSONStringToArg(argType, arg) + val, err := parseArgValue(atype, text) if err != nil { - return nil, err + return nil, fmt.Errorf("decoding parameter %q: %w", name, err) } - if ok { - values[i] = v - continue + vals[i+1] = val + } + return vals, nil +} + +func parseArgValue(atype reflect.Type, text string) (reflect.Value, error) { + // Regardless whether the argument is a pointer type, allocate a pointer so + // we can set the computed value. + var out reflect.Value + isPtr := atype.Kind() == reflect.Ptr + if isPtr { + out = reflect.New(atype.Elem()) + } else { + out = reflect.New(atype) + } + + baseType := out.Type().Elem() + if isIntType(baseType) { + // Integral type: Require a base-10 digit string. For compatibility with + // existing use allow quotation marks. + v, err := decodeInteger(text) + if err != nil { + return reflect.Value{}, fmt.Errorf("invalid integer: %w", err) } + out.Elem().Set(reflect.ValueOf(v).Convert(baseType)) + } else if isStringOrBytes(baseType) { + // String or byte slice: Check for quotes, hex encoding. + dec, err := decodeString(text) + if err != nil { + return reflect.Value{}, err + } + out.Elem().Set(reflect.ValueOf(dec).Convert(baseType)) - values[i], err = jsonStringToArg(argType, arg) + } else if baseType.Kind() == reflect.Bool { + b, err := strconv.ParseBool(text) if err != nil { - return nil, err + return reflect.Value{}, fmt.Errorf("invalid boolean: %w", err) } - } + out.Elem().Set(reflect.ValueOf(b)) - return values, nil -} + } else { + // We don't know how to represent other types. + return reflect.Value{}, fmt.Errorf("unsupported argument type %v", baseType) + } -func jsonStringToArg(rt reflect.Type, arg string) (reflect.Value, error) { - rv := reflect.New(rt) - err := tmjson.Unmarshal([]byte(arg), rv.Interface()) - if err != nil { - return rv, err + // If the argument wants a pointer, return the value as-is, otherwise + // indirect the pointer back off. + if isPtr { + return out, nil } - rv = rv.Elem() - return rv, nil + return out.Elem(), nil } -func nonJSONStringToArg(rt reflect.Type, arg string) (reflect.Value, bool, error) { - if rt.Kind() == reflect.Ptr { - rv1, ok, err := nonJSONStringToArg(rt.Elem(), arg) - switch { - case err != nil: - return reflect.Value{}, false, err - case ok: - rv := reflect.New(rt.Elem()) - rv.Elem().Set(rv1) - return rv, true, nil - default: - return reflect.Value{}, false, nil - } - } else { - return _nonJSONStringToArg(rt, arg) +var uint64Type = reflect.TypeOf(uint64(0)) + +// isIntType reports whether atype is an integer-shaped type. +func isIntType(atype reflect.Type) bool { + switch atype.Kind() { + case reflect.Float32, reflect.Float64: + return false + default: + return atype.ConvertibleTo(uint64Type) } } -// NOTE: rt.Kind() isn't a pointer. -func _nonJSONStringToArg(rt reflect.Type, arg string) (reflect.Value, bool, error) { - isIntString := reInt.Match([]byte(arg)) - isQuotedString := strings.HasPrefix(arg, `"`) && strings.HasSuffix(arg, `"`) - isHexString := strings.HasPrefix(strings.ToLower(arg), "0x") - - var expectingString, expectingByteSlice, expectingInt bool - switch rt.Kind() { - case reflect.Int, - reflect.Uint, - reflect.Int8, - reflect.Uint8, - reflect.Int16, - reflect.Uint16, - reflect.Int32, - reflect.Uint32, - reflect.Int64, - reflect.Uint64: - expectingInt = true +// isStringOrBytes reports whether atype is a string or []byte. +func isStringOrBytes(atype reflect.Type) bool { + switch atype.Kind() { case reflect.String: - expectingString = true + return true case reflect.Slice: - expectingByteSlice = rt.Elem().Kind() == reflect.Uint8 + return atype.Elem().Kind() == reflect.Uint8 + default: + return false } +} - if isIntString && expectingInt { - qarg := `"` + arg + `"` - rv, err := jsonStringToArg(rt, qarg) - if err != nil { - return rv, false, err - } - - return rv, true, nil - } - - if isHexString { - if !expectingString && !expectingByteSlice { - err := fmt.Errorf("got a hex string arg, but expected '%s'", - rt.Kind().String()) - return reflect.ValueOf(nil), false, err - } - - var value []byte - value, err := hex.DecodeString(arg[2:]) - if err != nil { - return reflect.ValueOf(nil), false, err - } - if rt.Kind() == reflect.String { - return reflect.ValueOf(string(value)), true, nil - } - return reflect.ValueOf(value), true, nil - } +// isQuotedString reports whether s is enclosed in double quotes. +func isQuotedString(s string) bool { + return len(s) >= 2 && strings.HasPrefix(s, `"`) && strings.HasSuffix(s, `"`) +} - if isQuotedString && expectingByteSlice { - v := reflect.New(reflect.TypeOf("")) - err := tmjson.Unmarshal([]byte(arg), v.Interface()) - if err != nil { - return reflect.ValueOf(nil), false, err - } - v = v.Elem() - return reflect.ValueOf([]byte(v.String())), true, nil +// decodeInteger decodes s into an int64. If s is "double quoted" the quotes +// are removed; otherwise s must be a base-10 digit string. +func decodeInteger(s string) (int64, error) { + if isQuotedString(s) { + s = s[1 : len(s)-1] } - - return reflect.ValueOf(nil), false, nil + return strconv.ParseInt(s, 10, 64) } -func getParam(r *http.Request, param string) string { - s := r.URL.Query().Get(param) - if s == "" { - s = r.FormValue(param) +// decodeString decodes s into a byte slice. If s has an 0x prefix, it is +// treated as a hex-encoded string. If it is "double quoted" it is treated as a +// JSON string value. Otherwise, s is converted to bytes directly. +func decodeString(s string) ([]byte, error) { + if lc := strings.ToLower(s); strings.HasPrefix(lc, "0x") { + return hex.DecodeString(lc[2:]) + } else if isQuotedString(s) { + var dec string + if err := json.Unmarshal([]byte(s), &dec); err != nil { + return nil, fmt.Errorf("invalid quoted string: %w", err) + } + return []byte(dec), nil } - return s + return []byte(s), nil } diff --git a/rpc/jsonrpc/server/parse_test.go b/rpc/jsonrpc/server/parse_test.go index 6533a5d44..9b222b507 100644 --- a/rpc/jsonrpc/server/parse_test.go +++ b/rpc/jsonrpc/server/parse_test.go @@ -187,8 +187,15 @@ func TestParseURI(t *testing.T) { // can parse numbers quoted, too {[]string{`"7"`, `"flew"`}, 7, "flew", false}, {[]string{`"-10"`, `"bob"`}, -10, "bob", false}, - // cant parse strings uquoted - {[]string{`"-10"`, `bob`}, -10, "bob", true}, + // can parse strings hex-escaped, in either case + {[]string{`-9`, `0x626f62`}, -9, "bob", false}, + {[]string{`-9`, `0X646F7567`}, -9, "doug", false}, + // can parse strings unquoted (as per OpenAPI docs) + {[]string{`0`, `hey you`}, 0, "hey you", false}, + // fail for invalid numbers, strings, hex + {[]string{`"-xx"`, `bob`}, 0, "", true}, // bad number + {[]string{`"95""`, `"bob`}, 0, "", true}, // bad string + {[]string{`15`, `0xa`}, 0, "", true}, // bad hex } for idx, tc := range cases { i := strconv.Itoa(idx) @@ -198,14 +205,14 @@ func TestParseURI(t *testing.T) { tc.raw[0], tc.raw[1]) req, err := http.NewRequest("GET", url, nil) assert.NoError(t, err) - vals, err := httpParamsToArgs(call, req) + vals, err := parseURLParams(context.Background(), call, req) if tc.fail { assert.Error(t, err, i) } else { assert.NoError(t, err, "%s: %+v", i, err) - if assert.Equal(t, 2, len(vals), i) { - assert.Equal(t, tc.height, vals[0].Int(), i) - assert.Equal(t, tc.name, vals[1].String(), i) + if assert.Equal(t, 3, len(vals), i) { + assert.Equal(t, tc.height, vals[1].Int(), i) + assert.Equal(t, tc.name, vals[2].String(), i) } } diff --git a/test/e2e/docker/Dockerfile b/test/e2e/docker/Dockerfile index 260df23f3..4e19fe9f8 100644 --- a/test/e2e/docker/Dockerfile +++ b/test/e2e/docker/Dockerfile @@ -1,7 +1,7 @@ # We need to build in a Linux environment to support C libraries, e.g. RocksDB. # We use Debian instead of Alpine, so that we can use binary database packages # instead of spending time compiling them. -FROM golang:1.16 +FROM golang:1.17 RUN apt-get -qq update -y && apt-get -qq upgrade -y >/dev/null RUN apt-get -qq install -y libleveldb-dev librocksdb-dev >/dev/null