internal/encoding/json/decode_number accepts string values that contain a comma

[MarTango]

What version of protobuf and what language are you using? Version: master, Go

What did you do?

With the parseNumber function:

func main() {
	n, ok := parseNumber([]byte("1234,anything can come after the comma"))
	if ok {
		panic(fmt.Sprintf("should not be ok, %d", n))
	}
}

panics with should not be ok, 4, indicating that it parsed "1234,anything can come after the comma" into the integer 1234, even though this should really throw an error.

What did you expect to see?

ok == false

What did you see instead?

ok == true

Anything else we should know about your project / environment? No

[dsnet]

The parseNumber function is internal to the package and only parses a JSON number out of the front of the input. Is this somehow resulting in a bug in the observable API for protojson?

[MarTango]

Yes, this propagates up to protojson level. For example if I have a protobuf

message HasInt {
  int64 value = 1;
}

I can do

v := &HasInt{}
protojson.Unmarshal([]byte(`{"value": "1,lol"}`, v)

which will succeed, and unmarshal such that v.Value == 1

[dsnet]

Thanks, I recommend next time filing the buggy observed behavior rather than pointing at a particular internal function. The behavior of parseNumber is correct. I suspect the problem is actually parseNumberParts, which should probably reject trailing data since it's only called from the context of a standalone token.

[kitami1976]

The same behavior is observed if the string contains spaces:

v := &HasInt{}
protojson.Unmarshal([]byte(`{"value": "1 2"}`, v)

I think the problem might be in the unmarshalInt(), unmarshalUint() and unmarshalFloat() methods in protobuf/encoding/protojson/decode.go. They all create a new decoder to decode the string value in case tok.Kind() == json.String, but dec.Read() doesn't necessarily read the entire string.

One possible fix is to add a second invocation to Read() to detect EOF:

tok1, err := dec.Read()
if err != nil || tok1.Kind() != json.EOF {
    return protoreflect.Value{}, false
}

[puellanivis]

🤔 I’m wondering if there would be some sort of a dec.More() function?