diff --git a/reflect/protodesc/editions.go b/reflect/protodesc/editions.go index f55b03695..697a61b29 100644 --- a/reflect/protodesc/editions.go +++ b/reflect/protodesc/editions.go @@ -11,6 +11,7 @@ import ( "google.golang.org/protobuf/internal/editiondefaults" "google.golang.org/protobuf/internal/filedesc" + "google.golang.org/protobuf/internal/genid" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/descriptorpb" @@ -128,23 +129,39 @@ func mergeEditionFeatures(parentDesc protoreflect.Descriptor, child *descriptorp // We must not use proto.GetExtension(child, gofeaturespb.E_Go) // because that only works for messages we generated, but not for // dynamicpb messages. See golang/protobuf#1669. + // + // Further, we harden this code against adversarial inputs: a + // service which accepts descriptors from a possibly malicious + // source shouldn't crash. goFeatures := child.ProtoReflect().Get(gofeaturespb.E_Go.TypeDescriptor()) if !goFeatures.IsValid() { return parentFS } + gf, ok := goFeatures.Interface().(protoreflect.Message) + if !ok { + return parentFS + } // gf.Interface() could be *dynamicpb.Message or *gofeaturespb.GoFeatures. - gf := goFeatures.Message() fields := gf.Descriptor().Fields() - if fd := fields.ByName("legacy_unmarshal_json_enum"); gf.Has(fd) { + if fd := fields.ByNumber(genid.GoFeatures_LegacyUnmarshalJsonEnum_field_number); fd != nil && + !fd.IsList() && + fd.Kind() == protoreflect.BoolKind && + gf.Has(fd) { parentFS.GenerateLegacyUnmarshalJSON = gf.Get(fd).Bool() } - if fd := fields.ByName("strip_enum_prefix"); gf.Has(fd) { + if fd := fields.ByNumber(genid.GoFeatures_StripEnumPrefix_field_number); fd != nil && + !fd.IsList() && + fd.Kind() == protoreflect.EnumKind && + gf.Has(fd) { parentFS.StripEnumPrefix = int(gf.Get(fd).Enum()) } - if fd := fields.ByName("api_level"); gf.Has(fd) { + if fd := fields.ByNumber(genid.GoFeatures_ApiLevel_field_number); fd != nil && + !fd.IsList() && + fd.Kind() == protoreflect.EnumKind && + gf.Has(fd) { parentFS.APILevel = int(gf.Get(fd).Enum()) } diff --git a/reflect/protodesc/editions_test.go b/reflect/protodesc/editions_test.go index 91c5f388f..9d9b769cf 100644 --- a/reflect/protodesc/editions_test.go +++ b/reflect/protodesc/editions_test.go @@ -7,41 +7,106 @@ package protodesc import ( "testing" + "google.golang.org/protobuf/internal/genid" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" "google.golang.org/protobuf/types/descriptorpb" "google.golang.org/protobuf/types/dynamicpb" "google.golang.org/protobuf/types/gofeaturespb" ) -func TestGoFeaturesDynamic(t *testing.T) { +func TestGoFeatures_NotExpectedType(t *testing.T) { md := (*gofeaturespb.GoFeatures)(nil).ProtoReflect().Descriptor() gf := dynamicpb.NewMessage(md) opaque := protoreflect.ValueOfEnum(gofeaturespb.GoFeatures_API_OPAQUE.Number()) - gf.Set(md.Fields().ByName("api_level"), opaque) - featureSet := &descriptorpb.FeatureSet{} + gf.Set(md.Fields().ByNumber(genid.GoFeatures_ApiLevel_field_number), opaque) dynamicExt := dynamicpb.NewExtensionType(gofeaturespb.E_Go.TypeDescriptor().Descriptor()) - proto.SetExtension(featureSet, dynamicExt, gf) - fd := &descriptorpb.FileDescriptorProto{ - Name: proto.String("a.proto"), - Dependency: []string{ - "google/protobuf/go_features.proto", - }, - Edition: descriptorpb.Edition_EDITION_2023.Enum(), - Syntax: proto.String("editions"), - Options: &descriptorpb.FileOptions{ - Features: featureSet, + extFile, err := NewFile(&descriptorpb.FileDescriptorProto{ + Name: proto.String("test.proto"), + Dependency: []string{"google/protobuf/descriptor.proto"}, + Extension: []*descriptorpb.FieldDescriptorProto{ + { + Name: proto.String("ext1002"), + Number: proto.Int32(int32(gofeaturespb.E_Go.TypeDescriptor().Number())), + Type: descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(), + Label: descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL.Enum(), + Extendee: proto.String(".google.protobuf.FeatureSet"), + }, }, + }, protoregistry.GlobalFiles) + if err != nil { + t.Fatal(err) + } + nonMessageExt := dynamicpb.NewExtensionType(extFile.Extensions().Get(0)) + + extFdProto := ToFieldDescriptorProto(gofeaturespb.E_Go.TypeDescriptor().Descriptor()) + extMsgProto := ToDescriptorProto(gofeaturespb.E_Go.TypeDescriptor().Descriptor().Message()) + extMsgProto.Field = extMsgProto.Field[:1] // remove all but first field from the message + extFile, err = NewFile(&descriptorpb.FileDescriptorProto{ + Name: proto.String("google/protobuf/go_features.proto"), + Package: proto.String("pb"), + Dependency: []string{"google/protobuf/descriptor.proto"}, + Extension: []*descriptorpb.FieldDescriptorProto{extFdProto}, + MessageType: []*descriptorpb.DescriptorProto{extMsgProto}, + }, protoregistry.GlobalFiles) + if err != nil { + t.Fatal(err) } - fds := &descriptorpb.FileDescriptorSet{ - File: []*descriptorpb.FileDescriptorProto{ - ToFileDescriptorProto(descriptorpb.File_google_protobuf_descriptor_proto), - ToFileDescriptorProto(gofeaturespb.File_google_protobuf_go_features_proto), - fd, + missingFieldsExt := dynamicpb.NewExtensionType(extFile.Extensions().Get(0)) + gfMissingFields := dynamicpb.NewMessage(extFile.Messages().Get(0)) + gfPresentField := gfMissingFields.Descriptor().Fields().Get(0) + gfMissingFields.Set(gfPresentField, gfMissingFields.NewField(gfPresentField)) + + testCases := []struct { + name string + ext protoreflect.ExtensionType + val any + }{ + { + name: "dynamic_message", + ext: dynamicExt, + val: gf, + }, + { + name: "not_a_message", + ext: nonMessageExt, + val: "abc", + }, + { + name: "message_missing_fields", + ext: missingFieldsExt, + val: gfMissingFields, }, } - if _, err := NewFiles(fds); err != nil { - t.Fatal(err) + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + featureSet := &descriptorpb.FeatureSet{} + proto.SetExtension(featureSet, tc.ext, tc.val) + + fd := &descriptorpb.FileDescriptorProto{ + Name: proto.String("a.proto"), + Dependency: []string{ + "google/protobuf/go_features.proto", + }, + Edition: descriptorpb.Edition_EDITION_2023.Enum(), + Syntax: proto.String("editions"), + Options: &descriptorpb.FileOptions{ + Features: featureSet, + }, + } + fds := &descriptorpb.FileDescriptorSet{ + File: []*descriptorpb.FileDescriptorProto{ + ToFileDescriptorProto(descriptorpb.File_google_protobuf_descriptor_proto), + ToFileDescriptorProto(gofeaturespb.File_google_protobuf_go_features_proto), + fd, + }, + } + if _, err := NewFiles(fds); err != nil { + t.Fatal(err) + } + }) } }