From beaa55256c574913ed48c983296269f23025c923 Mon Sep 17 00:00:00 2001 From: cybrcodr Date: Mon, 8 Jun 2020 22:27:39 -0700 Subject: [PATCH] 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 --- encoding/protojson/decode_test.go | 24 ++++++++++++++++++++++-- encoding/protojson/encode_test.go | 22 ++++++++++++++++++++-- encoding/protojson/well_known_types.go | 19 +++++++++++-------- internal/conformance/failing_tests.txt | 1 - 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/encoding/protojson/decode_test.go b/encoding/protojson/decode_test.go index 97fc5aff..5e3dcffb 100644 --- a/encoding/protojson/decode_test.go +++ b/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{ diff --git a/encoding/protojson/encode_test.go b/encoding/protojson/encode_test.go index 120c07a2..b4cf3bb4 100644 --- a/encoding/protojson/encode_test.go +++ b/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{}, diff --git a/encoding/protojson/well_known_types.go b/encoding/protojson/well_known_types.go index d26cd395..def7377c 100644 --- a/encoding/protojson/well_known_types.go +++ b/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 { - s = strings.TrimSpace(s) - // Convert to snake_case. Unlike encoding, no validation is done because - // it is not possible to know the original path names. - list.Append(pref.ValueOfString(strs.JSONSnakeCase(s))) + for _, s0 := range paths { + s := strs.JSONSnakeCase(s0) + if strings.Contains(s0, "_") || !pref.FullName(s).IsValid() { + return d.newError(tok.Pos(), "%v contains invalid path: %q", genid.FieldMask_Paths_field_fullname, s0) + } + list.Append(pref.ValueOfString(s)) } return nil } diff --git a/internal/conformance/failing_tests.txt b/internal/conformance/failing_tests.txt index 92dc3743..e69de29b 100644 --- a/internal/conformance/failing_tests.txt +++ b/internal/conformance/failing_tests.txt @@ -1 +0,0 @@ -Recommended.Proto3.JsonInput.FieldMaskInvalidCharacter