Repository: fxamacker/cbor (go)

Issue:
feature: Add decoder option (non-default) to allow decoding CBOR text string containing invalid UTF-8
This new option would allow users to override the default setting and decode CBOR text string containing invalid UTF-8.

The encoder allows encoding invalid UTF-8 to CBOR text string because CBOR RFC 8949 states:

> Generic CBOR encoders provide an application interface that allows the application to specify any well-formed value to be encoded as a CBOR data item, including simple values and tags unknown to the encoder.
>
>Even though CBOR attempts to minimize these cases, not all well-formed CBOR data is valid: for example, the encoded text string 0x62c0ae does not contain valid UTF-8 (because [RFC3629] requires always using the shortest form) and so is not a valid CBOR item. 

The decoder currently checks and prevents decoding CBOR text string containing invalid UTF-8.

Although I recommend specifying or using protocols that reject invalid UTF-8 strings in CBOR text strings, some scenarios might find it useful.

Pull request description:
Add decoding option to allow invalid UTF-8
#### Description 

Add DecOptions.UTF8 to specify how to decode invalid UTF-8:
- UTF8RejectInvalid (default) returns SemanticError on invalid UTF-8
- UTF8DecodeInvalid decodes invalid UTF-8

Since CBOR text containing invalid UTF-8 isn't valid, decoder by default
rejects it and returns error.

This option is backward compatible by default.

See RFC 8949 Section 5.3 (Validity of Items) for more info.

Closes #320

Patch:
diff --git a/decode.go b/decode.go
index 0d5f253..79ccaf8 100644
--- a/decode.go
+++ b/decode.go
@@ -257,6 +257,26 @@ func (ec ExtraDecErrorCond) valid() bool {
 	return ec < maxExtraDecError
 }
 
+// UTF8Mode option specifies if decoder should
+// decode CBOR Text containing invalid UTF-8 string.
+type UTF8Mode int
+
+const (
+	// UTF8RejectInvalid rejects CBOR Text containing
+	// invalid UTF-8 string.
+	UTF8RejectInvalid UTF8Mode = iota
+
+	// UTF8DecodeInvalid allows decoding CBOR Text containing
+	// invalid UTF-8 string.
+	UTF8DecodeInvalid
+
+	maxUTF8Mode
+)
+
+func (um UTF8Mode) valid() bool {
+	return um < maxUTF8Mode
+}
+
 // DecOptions specifies decoding options.
 type DecOptions struct {
 	// DupMapKey specifies whether to enforce duplicate map key.
@@ -295,6 +315,10 @@ type DecOptions struct {
 	// when unmarshalling CBOR into an empty interface value.
 	// By default, unmarshal uses map[interface{}]interface{}.
 	DefaultMapType reflect.Type
+
+	// UTF8 specifies if decoder should decode CBOR Text containing invalid UTF-8.
+	// By default, unmarshal rejects CBOR text containing invalid UTF-8.
+	UTF8 UTF8Mode
 }
 
 // DecMode returns DecMode with immutable options and no tags (safe for concurrency).
@@ -397,6 +421,9 @@ func (opts DecOptions) decMode() (*decMode, error) {
 	if opts.DefaultMapType != nil && opts.DefaultMapType.Kind() != reflect.Map {
 		return nil, fmt.Errorf("cbor: invalid DefaultMapType %s", opts.DefaultMapType)
 	}
+	if !opts.UTF8.valid() {
+		return nil, errors.New("cbor: invalid UTF8 " + strconv.Itoa(int(opts.UTF8)))
+	}
 	dm := decMode{
 		dupMapKey:         opts.DupMapKey,
 		timeTag:           opts.TimeTag,
@@ -408,6 +435,7 @@ func (opts DecOptions) decMode() (*decMode, error) {
 		intDec:            opts.IntDec,
 		extraReturnErrors: opts.ExtraReturnErrors,
 		defaultMapType:    opts.DefaultMapType,
+		utf8:              opts.UTF8,
 	}
 	return &dm, nil
 }
@@ -440,6 +468,7 @@ type decMode struct {
 	intDec            IntDecMode
 	extraReturnErrors ExtraDecErrorCond
 	defaultMapType    reflect.Type
+	utf8              UTF8Mode
 }
 
 var defaultDecMode, _ = DecOptions{}.decMode()
@@ -456,6 +485,7 @@ func (dm *decMode) DecOptions() DecOptions {
 		TagsMd:            dm.tagsMd,
 		IntDec:            dm.intDec,
 		ExtraReturnErrors: dm.extraReturnErrors,
+		UTF8:              dm.utf8,
 	}
 }
 
@@ -1064,7 +1094,7 @@ func (d *decoder) parseTextString() ([]byte, error) {
 	if ai != 31 {
 		b := d.data[d.off : d.off+int(val)]
 		d.off += int(val)
-		if !utf8.Valid(b) {
+		if d.dm.utf8 == UTF8RejectInvalid && !utf8.Valid(b) {
 			return nil, &SemanticError{"cbor: invalid UTF-8 string"}
 		}
 		return b, nil
@@ -1075,7 +1105,7 @@ func (d *decoder) parseTextString() ([]byte, error) {
 		_, _, val = d.getHead()
 		x := d.data[d.off : d.off+int(val)]
 		d.off += int(val)
-		if !utf8.Valid(x) {
+		if d.dm.utf8 == UTF8RejectInvalid && !utf8.Valid(x) {
 			for !d.foundBreak() {
 				d.skip() // Skip remaining chunk on error
 			}


Test patch:
diff --git a/decode_test.go b/decode_test.go
index 1eee4bb..34e2cd2 100644
--- a/decode_test.go
+++ b/decode_test.go
@@ -1383,33 +1383,157 @@ func TestInvalidCBORUnmarshal(t *testing.T) {
 	}
 }
 
-func TestInvalidUTF8TextString(t *testing.T) {
-	invalidUTF8TextStringTests := []struct {
+func TestValidUTF8String(t *testing.T) {
+	dmRejectInvalidUTF8, err := DecOptions{UTF8: UTF8RejectInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+	dmDecodeInvalidUTF8, err := DecOptions{UTF8: UTF8DecodeInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+
+	testCases := []struct {
+		name     string
+		cborData []byte
+		dm       DecMode
+		wantObj  interface{}
+	}{
+		{
+			name:     "with UTF8RejectInvalid",
+			cborData: hexDecode("6973747265616d696e67"),
+			dm:       dmRejectInvalidUTF8,
+			wantObj:  "streaming",
+		},
+		{
+			name:     "with UTF8DecodeInvalid",
+			cborData: hexDecode("6973747265616d696e67"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  "streaming",
+		},
+		{
+			name:     "indef length with UTF8RejectInvalid",
+			cborData: hexDecode("7f657374726561646d696e67ff"),
+			dm:       dmRejectInvalidUTF8,
+			wantObj:  "streaming",
+		},
+		{
+			name:     "indef length with UTF8DecodeInvalid",
+			cborData: hexDecode("7f657374726561646d696e67ff"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  "streaming",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Decode to empty interface
+			var i interface{}
+			err = tc.dm.Unmarshal(tc.cborData, &i)
+			if err != nil {
+				t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+			}
+			if !reflect.DeepEqual(i, tc.wantObj) {
+				t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, i, i, tc.wantObj, tc.wantObj)
+			}
+
+			// Decode to string
+			var v string
+			err = tc.dm.Unmarshal(tc.cborData, &v)
+			if err != nil {
+				t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+			}
+			if !reflect.DeepEqual(v, tc.wantObj) {
+				t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, v, v, tc.wantObj, tc.wantObj)
+			}
+		})
+	}
+}
+
+func TestInvalidUTF8String(t *testing.T) {
+	dmRejectInvalidUTF8, err := DecOptions{UTF8: UTF8RejectInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+	dmDecodeInvalidUTF8, err := DecOptions{UTF8: UTF8DecodeInvalid}.DecMode()
+	if err != nil {
+		t.Errorf("DecMode() returned an error %+v", err)
+	}
+
+	testCases := []struct {
 		name         string
 		cborData     []byte
+		dm           DecMode
+		wantObj      interface{}
 		wantErrorMsg string
 	}{
-		{"definite length text string", hexDecode("61fe"), invalidUTF8ErrorMsg},
-		{"indefinite length text string", hexDecode("7f62e6b061b4ff"), invalidUTF8ErrorMsg},
+		{
+			name:         "with UTF8RejectInvalid",
+			cborData:     hexDecode("61fe"),
+			dm:           dmRejectInvalidUTF8,
+			wantErrorMsg: invalidUTF8ErrorMsg,
+		},
+		{
+			name:     "with UTF8DecodeInvalid",
+			cborData: hexDecode("61fe"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  string([]byte{0xfe}),
+		},
+		{
+			name:         "indef length with UTF8RejectInvalid",
+			cborData:     hexDecode("7f62e6b061b4ff"),
+			dm:           dmRejectInvalidUTF8,
+			wantErrorMsg: invalidUTF8ErrorMsg,
+		},
+		{
+			name:     "indef length with UTF8DecodeInvalid",
+			cborData: hexDecode("7f62e6b061b4ff"),
+			dm:       dmDecodeInvalidUTF8,
+			wantObj:  string([]byte{0xe6, 0xb0, 0xb4}),
+		},
 	}
-	for _, tc := range invalidUTF8TextStringTests {
+
+	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			var i interface{}
-			if err := Unmarshal(tc.cborData, &i); err == nil {
-				t.Errorf("Unmarshal(0x%x) didn't return an error", tc.cborData)
-			} else if err.Error() != tc.wantErrorMsg {
-				t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+			// Decode to empty interface
+			var v interface{}
+			err = tc.dm.Unmarshal(tc.cborData, &v)
+			if tc.wantErrorMsg != "" {
+				if err == nil {
+					t.Errorf("Unmarshal(0x%x) didn't return error", tc.cborData)
+				} else if !strings.Contains(err.Error(), tc.wantErrorMsg) {
+					t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+				}
+				if !reflect.DeepEqual(v, tc.wantObj) {
+					t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, v, v, tc.wantObj, tc.wantObj)
+				}
 			}
 
+			// Decode to string
 			var s string
-			if err := Unmarshal(tc.cborData, &s); err == nil {
-				t.Errorf("Unmarshal(0x%x) didn't return an error", tc.cborData)
-			} else if err.Error() != tc.wantErrorMsg {
-				t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+			err = tc.dm.Unmarshal(tc.cborData, &s)
+			if tc.wantErrorMsg != "" {
+				if err == nil {
+					t.Errorf("Unmarshal(0x%x) didn't return error", tc.cborData)
+				} else if !strings.Contains(err.Error(), tc.wantErrorMsg) {
+					t.Errorf("Unmarshal(0x%x) error %q, want %q", tc.cborData, err.Error(), tc.wantErrorMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unmarshal(0x%x) returned error %q", tc.cborData, err)
+				}
+				if !reflect.DeepEqual(s, tc.wantObj) {
+					t.Errorf("Unmarshal(0x%x) returned %v (%T), want %v (%T)", tc.cborData, s, s, tc.wantObj, tc.wantObj)
+				}
 			}
 		})
 	}
+
 	// Test decoding of mixed invalid text string and valid text string
+	// with UTF8RejectInvalid option (default)
 	cborData := hexDecode("7f62e6b061b4ff7f657374726561646d696e67ff")
 	dec := NewDecoder(bytes.NewReader(cborData))
 	var s string
@@ -1423,6 +1547,20 @@ func TestInvalidUTF8TextString(t *testing.T) {
 	} else if s != "streaming" {
 		t.Errorf("Decode() returned %q, want %q", s, "streaming")
 	}
+
+	// Test decoding of mixed invalid text string and valid text string
+	// with UTF8DecodeInvalid option
+	dec = dmDecodeInvalidUTF8.NewDecoder(bytes.NewReader(cborData))
+	if err := dec.Decode(&s); err != nil {
+		t.Errorf("Decode() returned error %q", err)
+	} else if s != string([]byte{0xe6, 0xb0, 0xb4}) {
+		t.Errorf("Decode() returned %q, want %q", s, string([]byte{0xe6, 0xb0, 0xb4}))
+	}
+	if err := dec.Decode(&s); err != nil {
+		t.Errorf("Decode() returned error %v", err)
+	} else if s != "streaming" {
+		t.Errorf("Decode() returned %q, want %q", s, "streaming")
+	}
 }
 
 func TestUnmarshalStruct(t *testing.T) {
@@ -3063,15 +3201,16 @@ func TestUnmarshalToNotNilInterface(t *testing.T) {
 
 func TestDecOptions(t *testing.T) {
 	opts1 := DecOptions{
-		TimeTag:           DecTagRequired,
 		DupMapKey:         DupMapKeyEnforcedAPF,
-		IndefLength:       IndefLengthForbidden,
+		TimeTag:           DecTagRequired,
 		MaxNestedLevels:   100,
-		MaxMapPairs:       101,
 		MaxArrayElements:  102,
+		MaxMapPairs:       101,
+		IndefLength:       IndefLengthForbidden,
 		TagsMd:            TagsForbidden,
 		IntDec:            IntDecConvertSigned,
 		ExtraReturnErrors: ExtraDecErrorUnknownField,
+		UTF8:              UTF8DecodeInvalid,
 	}
 	dm, err := opts1.DecMode()
 	if err != nil {
@@ -4566,6 +4705,16 @@ func TestExtraErrorCondUnknowField(t *testing.T) {
 	}
 }
 
+func TestInvalidUTF8Mode(t *testing.T) {
+	wantErrorMsg := "cbor: invalid UTF8 2"
+	_, err := DecOptions{UTF8: 2}.DecMode()
+	if err == nil {
+		t.Errorf("DecMode() didn't return an error")
+	} else if err.Error() != wantErrorMsg {
+		t.Errorf("DecMode() returned error %q, want %q", err.Error(), wantErrorMsg)
+	}
+}
+
 func TestStreamExtraErrorCondUnknowField(t *testing.T) {
 	type s struct {
 		A string