From 6faf506540680e9d6953cfadee349f13f6411fe4 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Thu, 28 Apr 2022 15:18:11 -0700 Subject: [PATCH] rpc: fix byte string decoding for URL parameters (#8431) In #8397 I tried to remove all the cases where we needed to keep track of the target type of parameters for JSON encoding, but there is one case still left: When decoding parameters from URL query terms, there is no way to tell whether or not we need base64 encoding without knowing whether the underlying type of the target is string or []byte. To fix this, keep track of parameters that are []byte valued when RPCFunc is compiling its argument map, and use that when parsing URL query terms. Update the tests accordingly. --- rpc/jsonrpc/server/http_json_handler.go | 16 ++++---- rpc/jsonrpc/server/http_uri_handler.go | 26 ++++++++----- rpc/jsonrpc/server/parse_test.go | 26 ++++++++----- rpc/jsonrpc/server/rpc_func.go | 50 +++++++++++++++++-------- 4 files changed, 75 insertions(+), 43 deletions(-) diff --git a/rpc/jsonrpc/server/http_json_handler.go b/rpc/jsonrpc/server/http_json_handler.go index defcb7d9c..2eeded2d7 100644 --- a/rpc/jsonrpc/server/http_json_handler.go +++ b/rpc/jsonrpc/server/http_json_handler.go @@ -118,17 +118,15 @@ func writeListOfEndpoints(w http.ResponseWriter, r *http.Request, funcMap map[st noArgs := make(map[string]string) for name, rf := range funcMap { base := fmt.Sprintf("//%s/%s", r.Host, name) - // N.B. Check argNames, not args, since the type list includes the type - // of the leading context argument. - if len(rf.argNames) == 0 { + if len(rf.args) == 0 { noArgs[name] = base - } else { - query := append([]string(nil), rf.argNames...) - for i, arg := range query { - query[i] = arg + "=_" - } - hasArgs[name] = base + "?" + strings.Join(query, "&") + continue } + var query []string + for _, arg := range rf.args { + query = append(query, arg.name+"=_") + } + hasArgs[name] = base + "?" + strings.Join(query, "&") } w.Header().Set("Content-Type", "text/html") _ = listOfEndpoints.Execute(w, map[string]map[string]string{ diff --git a/rpc/jsonrpc/server/http_uri_handler.go b/rpc/jsonrpc/server/http_uri_handler.go index 7e1902ac1..c755bbaf1 100644 --- a/rpc/jsonrpc/server/http_uri_handler.go +++ b/rpc/jsonrpc/server/http_uri_handler.go @@ -23,7 +23,7 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit ctx := rpctypes.WithCallInfo(req.Context(), &rpctypes.CallInfo{ HTTPRequest: req, }) - args, err := parseURLParams(rpcFunc.argNames, req) + args, err := parseURLParams(rpcFunc.args, req) if err != nil { w.Header().Set("Content-Type", "text/plain") w.WriteHeader(http.StatusBadRequest) @@ -40,7 +40,7 @@ func makeHTTPHandler(rpcFunc *RPCFunc, logger log.Logger) func(http.ResponseWrit } } -func parseURLParams(argNames []string, req *http.Request) ([]byte, error) { +func parseURLParams(args []argInfo, req *http.Request) ([]byte, error) { if err := req.ParseForm(); err != nil { return nil, fmt.Errorf("invalid HTTP request: %w", err) } @@ -52,15 +52,15 @@ func parseURLParams(argNames []string, req *http.Request) ([]byte, error) { } params := make(map[string]interface{}) - for _, name := range argNames { - v, ok := getArg(name) + for _, arg := range args { + v, ok := getArg(arg.name) if !ok { continue } if z, err := decodeInteger(v); err == nil { - params[name] = z + params[arg.name] = z } else if b, err := strconv.ParseBool(v); err == nil { - params[name] = b + params[arg.name] = b } else if lc := strings.ToLower(v); strings.HasPrefix(lc, "0x") { dec, err := hex.DecodeString(lc[2:]) if err != nil { @@ -68,15 +68,23 @@ func parseURLParams(argNames []string, req *http.Request) ([]byte, error) { } else if len(dec) == 0 { return nil, errors.New("invalid empty hex string") } - params[name] = dec + if arg.isBinary { + params[arg.name] = dec + } else { + params[arg.name] = string(dec) + } } else if isQuotedString(v) { var dec string if err := json.Unmarshal([]byte(v), &dec); err != nil { return nil, fmt.Errorf("invalid quoted string: %w", err) } - params[name] = dec + if arg.isBinary { + params[arg.name] = []byte(dec) + } else { + params[arg.name] = dec + } } else { - params[name] = v + params[arg.name] = v } } return json.Marshal(params) diff --git a/rpc/jsonrpc/server/parse_test.go b/rpc/jsonrpc/server/parse_test.go index e6667fb0a..4a0e92ad1 100644 --- a/rpc/jsonrpc/server/parse_test.go +++ b/rpc/jsonrpc/server/parse_test.go @@ -188,44 +188,50 @@ func TestParseURI(t *testing.T) { tests := []struct { name string url string - args []string + args []argInfo want string fail bool }{ { name: "quoted numbers and strings", url: `http://localhost?num="7"&str="flew"&neg="-10"`, - args: []string{"neg", "num", "str", "other"}, + args: []argInfo{{name: "neg"}, {name: "num"}, {name: "str"}, {name: "other"}}, want: `{"neg":-10,"num":7,"str":"flew"}`, }, { name: "unquoted numbers and strings", url: `http://localhost?num1=7&str1=cabbage&num2=-199&str2=hey+you`, - args: []string{"num1", "num2", "str1", "str2", "other"}, + args: []argInfo{{name: "num1"}, {name: "num2"}, {name: "str1"}, {name: "str2"}, {name: "other"}}, want: `{"num1":7,"num2":-199,"str1":"cabbage","str2":"hey you"}`, }, { - name: "byte strings in hex", + name: "quoted byte strings", + url: `http://localhost?left="Fahrvergnügen"&right="Applesauce"`, + args: []argInfo{{name: "left", isBinary: true}, {name: "right", isBinary: false}}, + want: `{"left":"RmFocnZlcmduw7xnZW4=","right":"Applesauce"}`, + }, + { + name: "hexadecimal byte strings", url: `http://localhost?lower=0x626f62&upper=0X646F7567`, - args: []string{"upper", "lower", "other"}, - want: `{"lower":"Ym9i","upper":"ZG91Zw=="}`, + args: []argInfo{{name: "upper", isBinary: true}, {name: "lower", isBinary: false}, {name: "other"}}, + want: `{"lower":"bob","upper":"ZG91Zw=="}`, }, { name: "invalid hex odd length", url: `http://localhost?bad=0xa`, - args: []string{"bad", "superbad"}, + args: []argInfo{{name: "bad"}, {name: "superbad"}}, fail: true, }, { name: "invalid hex empty", url: `http://localhost?bad=0x`, - args: []string{"bad"}, + args: []argInfo{{name: "bad"}}, fail: true, }, { name: "invalid quoted string", url: `http://localhost?bad="double""`, - args: []string{"bad"}, + args: []argInfo{{name: "bad"}}, fail: true, }, } @@ -305,7 +311,7 @@ func TestParseURI(t *testing.T) { if err != nil { t.Fatalf("NewRequest for %q: %v", test.url, err) } - bits, err := parseURLParams(echo.argNames, hreq) + bits, err := parseURLParams(echo.args, hreq) if err != nil { t.Fatalf("Parse %#q: unexpected error: %v", test.url, err) } diff --git a/rpc/jsonrpc/server/rpc_func.go b/rpc/jsonrpc/server/rpc_func.go index 456d97bfc..8eba28728 100644 --- a/rpc/jsonrpc/server/rpc_func.go +++ b/rpc/jsonrpc/server/rpc_func.go @@ -32,11 +32,20 @@ func RegisterRPCFuncs(mux *http.ServeMux, funcMap map[string]*RPCFunc, logger lo // RPCFunc contains the introspected type information for a function. type RPCFunc struct { - f reflect.Value // underlying rpc function - param reflect.Type // the parameter struct, or nil - result reflect.Type // the non-error result type, or nil - argNames []string // name of each argument (for display) - ws bool // websocket only + f reflect.Value // underlying rpc function + param reflect.Type // the parameter struct, or nil + result reflect.Type // the non-error result type, or nil + args []argInfo // names and type information (for URL decoding) + ws bool // websocket only +} + +// argInfo records the name of a field, along with a bit to tell whether the +// value of the field requires binary data, having underlying type []byte. The +// flag is needed when decoding URL parameters, where we permit quoted strings +// to be passed for either argument type. +type argInfo struct { + name string + isBinary bool // value wants binary data } // Call parses the given JSON parameters and calls the function wrapped by rf @@ -96,12 +105,12 @@ func (rf *RPCFunc) adjustParams(data []byte) (json.RawMessage, error) { var args []json.RawMessage if err := json.Unmarshal(base, &args); err != nil { return nil, err - } else if len(args) != len(rf.argNames) { - return nil, fmt.Errorf("got %d arguments, want %d", len(args), len(rf.argNames)) + } else if len(args) != len(rf.args) { + return nil, fmt.Errorf("got %d arguments, want %d", len(args), len(rf.args)) } m := make(map[string]json.RawMessage) for i, arg := range args { - m[rf.argNames[i]] = arg + m[rf.args[i].name] = arg } return json.Marshal(m) } else if bytes.HasPrefix(base, []byte("{")) || bytes.Equal(base, []byte("null")) { @@ -180,12 +189,15 @@ func newRPCFunc(f interface{}) (*RPCFunc, error) { rtype = ft.Out(0) } - var argNames []string + var args []argInfo if ptype != nil { for i := 0; i < ptype.NumField(); i++ { field := ptype.Field(i) if tag := strings.SplitN(field.Tag.Get("json"), ",", 2)[0]; tag != "" && tag != "-" { - argNames = append(argNames, tag) + args = append(args, argInfo{ + name: tag, + isBinary: isByteArray(field.Type), + }) } else if tag == "-" { // If the tag is "-" the field should explicitly be ignored, even // if it is otherwise eligible. @@ -194,16 +206,19 @@ func newRPCFunc(f interface{}) (*RPCFunc, error) { // Note that this is an aesthetic choice; the standard decoder will // match without regard to case anyway. name := strings.ToLower(field.Name[:1]) + field.Name[1:] - argNames = append(argNames, name) + args = append(args, argInfo{ + name: name, + isBinary: isByteArray(field.Type), + }) } } } return &RPCFunc{ - f: fv, - param: ptype, - result: rtype, - argNames: argNames, + f: fv, + param: ptype, + result: rtype, + args: args, }, nil } @@ -225,3 +240,8 @@ func isNullOrEmpty(params json.RawMessage) bool { bytes.Equal(params, []byte("{}")) || bytes.Equal(params, []byte("[]")) } + +// isByteArray reports whether t is (equivalent to) []byte. +func isByteArray(t reflect.Type) bool { + return t.Kind() == reflect.Slice && t.Elem().Kind() == reflect.Uint8 +}