fix(jsonpb): eliminate non-deterministic behaviour in unmarshalling

jsonpb/jsonpb.go

@@ -829,6 +829,25 @@ func UnmarshalString(str string, pb proto.Message) error {
 	return new(Unmarshaler).Unmarshal(strings.NewReader(str), pb)
 }
 
+// Oneof members use a wrapper pointer as their presence bit, so explicit JSON
+// null must be filtered before allocating that wrapper for members that would
+// otherwise behave like unset fields.
+func nullIsUnsetForOneofValue(target reflect.Value) bool {
+	targetType := target.Type()
+	if targetType.Kind() == reflect.Ptr {
+		_, isJSONPBUnmarshaler := reflect.Zero(targetType).Interface().(JSONPBUnmarshaler)
+		return targetType != reflect.TypeOf(&types.Value{}) && !isJSONPBUnmarshaler
+	}
+
+	if target.CanAddr() {
+		if _, ok := target.Addr().Interface().(JSONPBUnmarshaler); ok {
+			return false
+		}
+	}
+
+	return true
+}
+
 // unmarshalValue converts/copies a value into the target.
 // prop may be nil.
 func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMessage, prop *proto.Properties) error {
@@ -1103,13 +1122,29 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
 		}
 		// Check for any oneof fields.
 		if len(jsonFields) > 0 {
-			for _, oop := range sprops.OneofTypes {
+			oneofKeys := make([]string, 0, len(sprops.OneofTypes))
+			for k := range sprops.OneofTypes {
+				oneofKeys = append(oneofKeys, k)
+			}
+			slices.Sort(oneofKeys)
+
+			setOneofFields := make(map[int]bool)
+			for _, key := range oneofKeys {
+				oop := sprops.OneofTypes[key]
 				raw, ok := consumeField(oop.Prop)
 				if !ok {
 					continue
 				}
 				nv := reflect.New(oop.Type.Elem())
+				if bytes.Equal(bytes.TrimSpace(raw), []byte("null")) && nullIsUnsetForOneofValue(nv.Elem().Field(0)) {
+					continue
+				}
+				if setOneofFields[oop.Field] {
+					return fmt.Errorf("field %q would overwrite already-set oneof %q in %v",
+						oop.Prop.OrigName, targetType.Field(oop.Field).Name, targetType)
+				}
 				target.Field(oop.Field).Set(nv)
+				setOneofFields[oop.Field] = true
 				if err := u.unmarshalValue(nv.Elem().Field(0), raw, oop.Prop); err != nil {
 					return err
 				}
@@ -1118,7 +1153,14 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
 		// Handle proto2 extensions.
 		if len(jsonFields) > 0 {
 			if ep, ok := target.Addr().Interface().(proto.Message); ok {
-				for _, ext := range proto.RegisteredExtensions(ep) {
+				extensions := proto.RegisteredExtensions(ep)
+				extIDs := make([]int32, 0, len(extensions))
+				for id := range extensions {
+					extIDs = append(extIDs, id)
+				}
+				sort.Sort(int32Slice(extIDs))
+				for _, id := range extIDs {
+					ext := extensions[id]
 					name := fmt.Sprintf("[%s]", ext.Name)
 					raw, ok := jsonFields[name]
 					if !ok {

jsonpb/jsonpb_test.go

@@ -961,6 +961,8 @@ var unmarshalingShouldError = []struct {
 	{"StringValue containing invalid character", `{"str": "\U00004E16\U0000754C"}`, &pb.KnownTypes{}},
 	{"StructValue containing invalid character", `{"str": "\U00004E16\U0000754C"}`, &types.Struct{}},
 	{"repeated proto3 enum with non array input", `{"rFunny":"PUNS"}`, &proto3pb.Message{RFunny: []proto3pb.Message_Humour{}}},
+	{"oneof conflict: two fields from the same group", `{"title":"foo","salary":31000}`, new(pb.MsgWithOneof)},
+	{"oneof conflict: camelCase and orig_name from the same group", `{"Country":"Australia","homeAddress":"Brisbane"}`, new(pb.MsgWithOneof)},
 }
 
 func TestUnmarshalingBadInput(t *testing.T) {
@@ -972,6 +974,66 @@ func TestUnmarshalingBadInput(t *testing.T) {
 	}
 }
 
+func TestUnmarshalOneofNullIsIgnored(t *testing.T) {
+	tests := []struct {
+		desc string
+		in   string
+		want *pb.MsgWithOneof
+	}{
+		{
+			desc: "single null member",
+			in:   `{"title":null}`,
+			want: &pb.MsgWithOneof{},
+		},
+		{
+			desc: "null member with concrete sibling",
+			in:   `{"title":null,"salary":31000}`,
+			want: &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Salary{Salary: 31000}},
+		},
+		{
+			desc: "concrete sibling with null member",
+			in:   `{"salary":31000,"title":null}`,
+			want: &pb.MsgWithOneof{Union: &pb.MsgWithOneof_Salary{Salary: 31000}},
+		},
+	}
+
+	for _, tt := range tests {
+		var msg pb.MsgWithOneof
+		if err := UnmarshalString(tt.in, &msg); err != nil {
+			t.Fatalf("%s: %v", tt.desc, err)
+		}
+
+		got := proto.MarshalTextString(&msg)
+		want := proto.MarshalTextString(tt.want)
+		if got != want {
+			t.Errorf("%s: got [%s] want [%s]", tt.desc, got, want)
+		}
+	}
+}
+
+// TestUnmarshalOneofConflictDeterminism verifies that unmarshalling a JSON
+// object containing multiple keys from the same oneof group always produces a
+// consistent result. Before the fix the outcome depended on random map
+// iteration order, so different runs could decode different oneof variants.
+func TestUnmarshalOneofConflictDeterminism(t *testing.T) {
+	const runs = 100
+	seen := make(map[string]struct{})
+	for range runs {
+		var msg pb.MsgWithOneof
+		err := UnmarshalString(`{"title":"foo","salary":31000}`, &msg)
+		var key string
+		if err != nil {
+			key = "error:" + err.Error()
+		} else {
+			key = proto.MarshalTextString(&msg)
+		}
+		seen[key] = struct{}{}
+	}
+	if len(seen) > 1 {
+		t.Errorf("non-deterministic unmarshal: got %d different outcomes over %d runs", len(seen), runs)
+	}
+}
+
 type funcResolver func(turl string) (proto.Message, error)
 
 func (fn funcResolver) Resolve(turl string) (proto.Message, error) {