graphql-scalars icon indicating copy to clipboard operation
graphql-scalars copied to clipboard

Dangerous and incorrect int and float validation and parsing

Open Jiia opened this issue 3 years ago • 2 comments

The int and float validation made by this library are not working correctly. They not only fail to validate the input values but they also incorrectly parse and modify the input values.

https://github.com/Urigo/graphql-scalars/blob/master/src/scalars/utilities.ts#L100

For example if your field is of type float and your input value is eg string 0,8 what would you expect to happen?

You probably would expect that a string doesn't pass validation for a field of type float, but you'd be wrong. The string 0,8 passes validation.

Because the library uses parseFloat the input value will be changed to the number 0 once the library is done with it.

I think the int and float validation should be reworked to not use any sort of parsing.

Jiia avatar Jun 21 '21 08:06 Jiia

The scenario and your request are different. We can solve that issue by comparing the final coerced value with the input value but this is completely unrelated to removing the parsing stuff. Even if we do the solution I said above, I want to stick with graphql-js scalars' logic https://github.com/graphql/graphql-js/blob/df1bddac6e7f2cef730b5c3695126820d075bbfd/src/type/scalars.ts#L118

ardatan avatar Jun 21 '21 08:06 ardatan

I touched on this with #479 but was told it doesn't exist. lt does, but I haven't had time to produce a unit test to prove it. I'm not sure about things like "0,8", but I can say for sure that it is not doing "strong" typing as you would want with GraphQL. Instead, it is using JavaScript type coercion which cuts the legs out from under GraphQL typing a bit. A value like "3.1415" submitted on a PositiveInt type would be allowed and become a number type containing 3.

robross0606 avatar Jul 20 '21 16:07 robross0606