Variable value validation allows String values for Int/Float variables and vice-versa

[BrunoScheufler]

We observed that the VariableValues function of the validator and more specifically validateVarType allows to pass variable values of different types than expected in a few cases.

Examples

Example 1

When supplying a variable of type String, the value could be numeric.

mutation ($var: String) {
   doSomething(stringField: $var)
}

with variables

{
  "var": 1
}

Example 2

When supplying a variable of type Int/Float, the value could be a string.

mutation ($var: Int) {
   doSomething(intField $var)
}

with variables

{
  "var": "some string"
}

Is this behaviour intended?

The most important question here is whether this is intended to be the case, which could very well work if the final value is transformed in a later piece of logic. However, it seemed counterintuitive to allow type mismatch in a function that verifies that types do line up.

Source of the (potential) bug

As the scalar validation makes heavy use of reflection, based on the variable type declared in the operation, the validator checks against the kind of the provided value and makes sure that for a specific GraphQL scalar type only allows specific value kinds.

This one-sided check already becomes the problem in our case: As numeric values are unmarshalled to be json.Number values, which are stored as strings underneath, kind will evaluate to a string for numeric values.

In the validation case for String variables, numerics will pass the kind check in that way and won't get caught. This implies that we can add another check based on the actual type of the value, to make sure no json.Number is passed for a String variable. We introduced the following change for String scalar validation:

case "String":
  if kind == reflect.String {
    if val.Type().String() == "json.Number" {
      return gqlerror.ErrorPathf(v.path, "cannot use number as String")
    }

    return nil
  }

When validating the built-in numeric scalars (Int and Float), we naturally allow values with the kind String (for the reason that json.Number values are essentially Strings), but don't prevent "regular" String values from being supplied. For this case, we've added the following check:

case "String":
  if kind == reflect.String {
    if val.Type().String() == "json.Number" {
      return gqlerror.ErrorPathf(v.path, "cannot use number as String")
    }

    return nil
  }

Conclusion

Depending on the outcome of the question whether this is intended or not, I'd like to contribute a fix (could be the one above, or a different one with the same outcome).