encoding/protojson: add better validation to FieldMask serialization

For marshaling, apart from already existing check that each item in
paths field is reversible, also make sure that string is a valid
protobuf name.

For unmarshaling, make sure that each resulting item in paths field is
a valid protobuf name and input path item does not contain _. The latter
check satisfies the conformance test
Recommended.Proto3.JsonInput.FieldMaskInvalidCharacter.

Fixes golang/protobuf#1141.

Change-Id: Iffc278089b20e496b7216d5b8c966b21b70e782d
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/236998
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>

encoding/protojson/decode_test.go

@@ -1903,7 +1903,7 @@ func TestUnmarshal(t *testing.T) {
 	}, {
 		desc:         "FieldMask",
 		inputMessage: &fieldmaskpb.FieldMask{},
-		inputText:    `"foo,fooBar , foo.barQux ,Foo"`,
+		inputText:    `"foo,fooBar,foo.barQux,Foo"`,
 		wantMessage: &fieldmaskpb.FieldMask{
 			Paths: []string{
 				"foo",
@@ -1912,11 +1912,31 @@ func TestUnmarshal(t *testing.T) {
 				"_foo",
 			},
 		},
+	}, {
+		desc:         "FieldMask empty path 1",
+		inputMessage: &fieldmaskpb.FieldMask{},
+		inputText:    `"foo,"`,
+		wantErr:      `google.protobuf.FieldMask.paths contains invalid path: ""`,
+	}, {
+		desc:         "FieldMask empty path 2",
+		inputMessage: &fieldmaskpb.FieldMask{},
+		inputText:    `"foo,  ,bar"`,
+		wantErr:      `google.protobuf.FieldMask.paths contains invalid path: "  "`,
+	}, {
+		desc:         "FieldMask invalid char 1",
+		inputMessage: &fieldmaskpb.FieldMask{},
+		inputText:    `"foo_bar"`,
+		wantErr:      `google.protobuf.FieldMask.paths contains invalid path: "foo_bar"`,
+	}, {
+		desc:         "FieldMask invalid char 2",
+		inputMessage: &fieldmaskpb.FieldMask{},
+		inputText:    `"foo@bar"`,
+		wantErr:      `google.protobuf.FieldMask.paths contains invalid path: "foo@bar"`,
 	}, {
 		desc:         "FieldMask field",
 		inputMessage: &pb2.KnownTypes{},
 		inputText: `{
-  "optFieldmask": "foo, qux.fooBar"
+  "optFieldmask": "foo,qux.fooBar"
 }`,
 		wantMessage: &pb2.KnownTypes{
 			OptFieldmask: &fieldmaskpb.FieldMask{

encoding/protojson/encode_test.go

@@ -1507,17 +1507,35 @@ func TestMarshal(t *testing.T) {
 		},
 		want: `"foo,fooBar,foo.barQux,Foo"`,
 	}, {
-		desc: "FieldMask error 1",
+		desc: "FieldMask empty string path",
+		input: &fieldmaskpb.FieldMask{
+			Paths: []string{""},
+		},
+		wantErr: true,
+	}, {
+		desc: "FieldMask path contains spaces only",
+		input: &fieldmaskpb.FieldMask{
+			Paths: []string{"  "},
+		},
+		wantErr: true,
+	}, {
+		desc: "FieldMask irreversible error 1",
 		input: &fieldmaskpb.FieldMask{
 			Paths: []string{"foo_"},
 		},
 		wantErr: true,
 	}, {
-		desc: "FieldMask error 2",
+		desc: "FieldMask irreversible error 2",
 		input: &fieldmaskpb.FieldMask{
 			Paths: []string{"foo__bar"},
 		},
 		wantErr: true,
+	}, {
+		desc: "FieldMask invalid char",
+		input: &fieldmaskpb.FieldMask{
+			Paths: []string{"foo@bar"},
+		},
+		wantErr: true,
 	}, {
 		desc:  "Any empty",
 		input: &anypb.Any{},

encoding/protojson/well_known_types.go

@@ -16,7 +16,6 @@ import (
 	"google.golang.org/protobuf/internal/genid"
 	"google.golang.org/protobuf/internal/strs"
 	"google.golang.org/protobuf/proto"
-	"google.golang.org/protobuf/reflect/protoreflect"
 	pref "google.golang.org/protobuf/reflect/protoreflect"
 )
 
@@ -24,7 +23,7 @@ type marshalFunc func(encoder, pref.Message) error
 
 // wellKnownTypeMarshaler returns a marshal function if the message type
 // has specialized serialization behavior. It returns nil otherwise.
-func wellKnownTypeMarshaler(name protoreflect.FullName) marshalFunc {
+func wellKnownTypeMarshaler(name pref.FullName) marshalFunc {
 	if name.Parent() == genid.GoogleProtobuf_package {
 		switch name.Name() {
 		case genid.Any_message_name:
@@ -62,7 +61,7 @@ type unmarshalFunc func(decoder, pref.Message) error
 
 // wellKnownTypeUnmarshaler returns a unmarshal function if the message type
 // has specialized serialization behavior. It returns nil otherwise.
-func wellKnownTypeUnmarshaler(name protoreflect.FullName) unmarshalFunc {
+func wellKnownTypeUnmarshaler(name pref.FullName) unmarshalFunc {
 	if name.Parent() == genid.GoogleProtobuf_package {
 		switch name.Name() {
 		case genid.Any_message_name:
@@ -843,6 +842,9 @@ func (e encoder) marshalFieldMask(m pref.Message) error {
 
 	for i := 0; i < list.Len(); i++ {
 		s := list.Get(i).String()
+		if !pref.FullName(s).IsValid() {
+			return errors.New("%s contains invalid path: %q", genid.FieldMask_Paths_field_fullname, s)
+		}
 		// Return error if conversion to camelCase is not reversible.
 		cc := strs.JSONCamelCase(s)
 		if s != strs.JSONSnakeCase(cc) {
@@ -872,11 +874,12 @@ func (d decoder) unmarshalFieldMask(m pref.Message) error {
 	fd := m.Descriptor().Fields().ByNumber(genid.FieldMask_Paths_field_number)
 	list := m.Mutable(fd).List()
 
-	for _, s := range paths {
+	for _, s0 := range paths {
-		s = strings.TrimSpace(s)
+		s := strs.JSONSnakeCase(s0)
-		// Convert to snake_case. Unlike encoding, no validation is done because
+		if strings.Contains(s0, "_") || !pref.FullName(s).IsValid() {
-		// it is not possible to know the original path names.
+			return d.newError(tok.Pos(), "%v contains invalid path: %q", genid.FieldMask_Paths_field_fullname, s0)
-		list.Append(pref.ValueOfString(strs.JSONSnakeCase(s)))
+		}
+		list.Append(pref.ValueOfString(s))
 	}
 	return nil
 }

internal/conformance/failing_tests.txt

@@ -1 +0,0 @@
-Recommended.Proto3.JsonInput.FieldMaskInvalidCharacter