From 9d1327d0d0fb243a6a94627b56477b4e74b48484 Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Tue, 7 Jul 2020 16:19:28 -0400 Subject: [PATCH 1/6] Handle hex strings for bytesX types --- signer/core/signed_data.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go index 26ad0b63ffff..aaecf0b13968 100644 --- a/signer/core/signed_data.go +++ b/signer/core/signed_data.go @@ -481,6 +481,21 @@ func (typedData *TypedData) EncodeData(primaryType string, data map[string]inter return buffer.Bytes(), nil } +// Attempt to parse bytes in different formats: byte array, hex string, hexutil.Bytes. +func parseBytes(encType interface{}) ([]byte, bool) { + switch v := encType.(type) { + case []byte: + return v, true + case hexutil.Bytes: + return []byte(v), true + case string: + bytes, err := hexutil.Decode(v) + return bytes, err == nil + default: + return nil, false + } +} + func parseInteger(encType string, encValue interface{}) (*big.Int, error) { var ( length int @@ -560,7 +575,7 @@ func (typedData *TypedData) EncodePrimitiveValue(encType string, encValue interf } return crypto.Keccak256([]byte(strVal)), nil case "bytes": - bytesValue, ok := encValue.([]byte) + bytesValue, ok := parseBytes(encValue) if !ok { return nil, dataMismatchError(encType, encValue) } @@ -575,7 +590,7 @@ func (typedData *TypedData) EncodePrimitiveValue(encType string, encValue interf if length < 0 || length > 32 { return nil, fmt.Errorf("invalid size on bytes: %d", length) } - if byteValue, ok := encValue.(hexutil.Bytes); !ok { + if byteValue, ok := parseBytes(encValue); !ok { return nil, dataMismatchError(encType, encValue) } else { return math.PaddedBigBytes(new(big.Int).SetBytes(byteValue), 32), nil From ea8102078eac9c7888e797e9138de17c288cce0a Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Tue, 7 Jul 2020 16:19:38 -0400 Subject: [PATCH 2/6] Add tests for parseBytes --- signer/core/signed_data_internal_test.go | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go index 0d59fcfca893..4e6e1770847e 100644 --- a/signer/core/signed_data_internal_test.go +++ b/signer/core/signed_data_internal_test.go @@ -17,10 +17,42 @@ package core import ( + "fmt" "math/big" "testing" + + "github.com/ethereum/go-ethereum/common/hexutil" ) +func TestParseBytes(t *testing.T) { + for i, tt := range []struct { + v interface{} + exp []byte + }{ + {"0x", []byte{}}, + {"0x1234", []byte{0x12, 0x34}}, + {[]byte{12, 34}, []byte{12, 34}}, + {hexutil.Bytes([]byte{12, 34}), []byte{12, 34}}, + {"not a hex string", nil}, + {15, nil}, + {nil, nil}, + } { + out, ok := parseBytes(tt.v) + if tt.exp == nil { + if ok { + t.Errorf("Case %d: expecting !ok, got ok with %v", i, out) + } + continue + } + if !ok { + t.Errorf("Case %d: expecting ok got !ok", i) + } + if fmt.Sprintf("%#v", out) != fmt.Sprintf("%#v", tt.exp) { + t.Errorf("Case %d: expecting %v got %v", i, tt.exp, out) + } + } +} + func TestParseInteger(t *testing.T) { for i, tt := range []struct { t string From 9bae003db3945aa9acec825077c313e72361bdfd Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Mon, 27 Jul 2020 12:02:42 -0400 Subject: [PATCH 3/6] Improve tests --- signer/core/signed_data_internal_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go index 4e6e1770847e..e88d43166c3a 100644 --- a/signer/core/signed_data_internal_test.go +++ b/signer/core/signed_data_internal_test.go @@ -17,7 +17,7 @@ package core import ( - "fmt" + "bytes" "math/big" "testing" @@ -33,6 +33,8 @@ func TestParseBytes(t *testing.T) { {"0x1234", []byte{0x12, 0x34}}, {[]byte{12, 34}, []byte{12, 34}}, {hexutil.Bytes([]byte{12, 34}), []byte{12, 34}}, + {"1234", nil}, // not a proper hex-string + {"0x01233", nil}, // nibbles should be rejected {"not a hex string", nil}, {15, nil}, {nil, nil}, @@ -40,15 +42,15 @@ func TestParseBytes(t *testing.T) { out, ok := parseBytes(tt.v) if tt.exp == nil { if ok { - t.Errorf("Case %d: expecting !ok, got ok with %v", i, out) + t.Errorf("Case %d: expecting !ok, got ok with %x", i, out) } continue } if !ok { t.Errorf("Case %d: expecting ok got !ok", i) } - if fmt.Sprintf("%#v", out) != fmt.Sprintf("%#v", tt.exp) { - t.Errorf("Case %d: expecting %v got %v", i, tt.exp, out) + if !bytes.Equal(out, tt.exp) { + t.Errorf("Case %d: expecting %x got %x", i, tt.exp, out) } } } From 9e876ea241779d12cca611913e55e1e0b4f8e585 Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Mon, 27 Jul 2020 12:12:35 -0400 Subject: [PATCH 4/6] Return nil bytes if error is non-nil --- signer/core/signed_data.go | 5 ++++- signer/core/signed_data_internal_test.go | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go index aaecf0b13968..da8c17739dad 100644 --- a/signer/core/signed_data.go +++ b/signer/core/signed_data.go @@ -490,7 +490,10 @@ func parseBytes(encType interface{}) ([]byte, bool) { return []byte(v), true case string: bytes, err := hexutil.Decode(v) - return bytes, err == nil + if err != nil { + return nil, false + } + return bytes, true default: return nil, false } diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go index e88d43166c3a..181e3eaffbcc 100644 --- a/signer/core/signed_data_internal_test.go +++ b/signer/core/signed_data_internal_test.go @@ -41,8 +41,8 @@ func TestParseBytes(t *testing.T) { } { out, ok := parseBytes(tt.v) if tt.exp == nil { - if ok { - t.Errorf("Case %d: expecting !ok, got ok with %x", i, out) + if ok || out != nil { + t.Errorf("Case %d: expecting !ok, got ok = %v with out = %x", i, ok, out) } continue } From c9a70f8123facd7ffd71bba371416b97c1d7d984 Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Mon, 27 Jul 2020 12:18:18 -0400 Subject: [PATCH 5/6] Right-pad instead of left-pad bytes --- signer/core/signed_data.go | 7 +++- signer/core/signed_data_internal_test.go | 50 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go index da8c17739dad..4716a7863e4c 100644 --- a/signer/core/signed_data.go +++ b/signer/core/signed_data.go @@ -593,10 +593,13 @@ func (typedData *TypedData) EncodePrimitiveValue(encType string, encValue interf if length < 0 || length > 32 { return nil, fmt.Errorf("invalid size on bytes: %d", length) } - if byteValue, ok := parseBytes(encValue); !ok { + if byteValue, ok := parseBytes(encValue); !ok || len(byteValue) != length { return nil, dataMismatchError(encType, encValue) } else { - return math.PaddedBigBytes(new(big.Int).SetBytes(byteValue), 32), nil + // Right-pad the bits + dst := make([]byte, 32) + copy(dst, byteValue) + return dst, nil } } if strings.HasPrefix(encType, "int") || strings.HasPrefix(encType, "uint") { diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go index 181e3eaffbcc..be55bff3d6f2 100644 --- a/signer/core/signed_data_internal_test.go +++ b/signer/core/signed_data_internal_test.go @@ -24,6 +24,56 @@ import ( "github.com/ethereum/go-ethereum/common/hexutil" ) +func TestBytesPadding(t *testing.T) { + tests := []struct { + Type string + Input []byte + Output []byte // nil => error + }{ + { + // Fail on wrong length + Type: "bytes20", + Input: []byte{}, + Output: nil, + }, + { + Type: "bytes1", + Input: []byte{1}, + Output: []byte{1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + }, + { + Type: "bytes7", + Input: []byte{1, 2, 3, 4, 5, 6, 7}, + Output: []byte{1, 2, 3, 4, 5, 6, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + }, + { + Type: "bytes32", + Input: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}, + Output: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}, + }, + } + + d := TypedData{} + for _, test := range tests { + val, err := d.EncodePrimitiveValue(test.Type, test.Input, 1) + if test.Output == nil { + if err == nil { + t.Errorf("expected error, got nil error with result %v: case %v", val, test) + } + } else { + if err != nil { + t.Errorf("expected nil error, got %v: case %v", err, test) + } + if len(val) != 32 { + t.Errorf("expected len 32, got %v: case %v", len(val), test) + } + if fmt.Sprint(val) != fmt.Sprint(test.Output) { + t.Errorf("expected %x, got %x: case %v", test.Output, val, test) + } + } + } +} + func TestParseBytes(t *testing.T) { for i, tt := range []struct { v interface{} From 7a621bd864cc76427d41ebcd75c26860b1e2a237 Mon Sep 17 00:00:00 2001 From: Natsu Kagami Date: Mon, 3 Aug 2020 11:32:06 -0400 Subject: [PATCH 6/6] More tests --- signer/core/signed_data_internal_test.go | 28 ++++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go index be55bff3d6f2..9768ee0b3fbb 100644 --- a/signer/core/signed_data_internal_test.go +++ b/signer/core/signed_data_internal_test.go @@ -41,6 +41,11 @@ func TestBytesPadding(t *testing.T) { Input: []byte{1}, Output: []byte{1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, }, + { + Type: "bytes1", + Input: []byte{1, 2}, + Output: nil, + }, { Type: "bytes7", Input: []byte{1, 2, 3, 4, 5, 6, 7}, @@ -51,24 +56,29 @@ func TestBytesPadding(t *testing.T) { Input: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}, Output: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32}, }, + { + Type: "bytes32", + Input: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33}, + Output: nil, + }, } d := TypedData{} - for _, test := range tests { + for i, test := range tests { val, err := d.EncodePrimitiveValue(test.Type, test.Input, 1) if test.Output == nil { if err == nil { - t.Errorf("expected error, got nil error with result %v: case %v", val, test) + t.Errorf("test %d: expected error, got no error (result %x)", i, val) } } else { if err != nil { - t.Errorf("expected nil error, got %v: case %v", err, test) + t.Errorf("test %d: expected no error, got %v", i, err) } if len(val) != 32 { - t.Errorf("expected len 32, got %v: case %v", len(val), test) + t.Errorf("test %d: expected len 32, got %d", i, len(val)) } - if fmt.Sprint(val) != fmt.Sprint(test.Output) { - t.Errorf("expected %x, got %x: case %v", test.Output, val, test) + if !bytes.Equal(val, test.Output) { + t.Errorf("test %d: expected %x, got %x", i, test.Output, val) } } } @@ -92,15 +102,15 @@ func TestParseBytes(t *testing.T) { out, ok := parseBytes(tt.v) if tt.exp == nil { if ok || out != nil { - t.Errorf("Case %d: expecting !ok, got ok = %v with out = %x", i, ok, out) + t.Errorf("test %d: expected !ok, got ok = %v with out = %x", i, ok, out) } continue } if !ok { - t.Errorf("Case %d: expecting ok got !ok", i) + t.Errorf("test %d: expected ok got !ok", i) } if !bytes.Equal(out, tt.exp) { - t.Errorf("Case %d: expecting %x got %x", i, tt.exp, out) + t.Errorf("test %d: expected %x got %x", i, tt.exp, out) } } }