Handle JSON Number in ParseCommaStringSlice

[ccapurso]

The ParseCommaStringSlice function in parseutil performs a type assertion (i.e. in.(string)) but does not return an error if the assertion fails. Without an early return, a panic might occur in the mapstructure.NewDecoder call. StringToSliceHookFunc in mapstructure returns a hook func that performs a similar string type assertion but does not verify that it was successful. This can occur if the input is a json.Number given that its reflect.Kind is string. The type assertion in mapstructure will panic with invalid type assertion: data.(string) (non-interface type json.Number on left).

Unit tests for ParseCommaStringSlice were also added as there weren't any prior to this PR.

[briankassouf]

I believe that prior to this change non-strings were valid inputs: https://go.dev/play/p/oy2jtFp-k7z

Do we instead want to special case json.Number?

[ccapurso]

I believe that prior to this change non-strings were valid inputs: https://go.dev/play/p/oy2jtFp-k7z

Do we instead want to special case json.Number?

Thanks for pointing this out @briankassouf. The ParseCommaStringSlice function is a lot more permissive than I expected. I imagine that is due to setting WeaklyTypedInput to true in the mapstructure.DecoderConfig. For example, passing []bool{true, false, true} results in []string{"1", "0", "1"}. It's kind of interesting that this function has somewhat undefined/magical behavior with the weak typing. Based on this, it definitely makes sense to just special case json.Number. I can expand the unit tests but do we actually intend to make this function this permissive? The name (and associated use of StringToSliceHookFunc) doesn't necessarily imply that. Just want to ensure that our test cases cover what we do want to support.

[briankassouf]

@ccapurso Since we use it in our API parameter parsing code, i think it's hard to say if making it less permissive would break existing APIs usage or not. I'm also not sure if boundary is using it in any meaningful way. Since it takes an interface{} as input it's probably best to not break the existing contract and just special case json.Number

[ccapurso]

@briankassouf, definitely agree. I didn't mean to imply that we should be less permissive. I am trying to get a feel for whether there might be other cases that might panic so that we can handle accordingly and ensure that there are appropriate tests.

[hashicorp-cla]

All committers have signed the CLA.