From d22385cc93f8c7fe0558a9e47d7c54b428a71fdb Mon Sep 17 00:00:00 2001 From: Derek Lavigne Date: Mon, 18 Sep 2017 11:49:05 -0400 Subject: [PATCH] Update query variable coercion to meet the rules outlined in the specification. The framework currently coerces query variables similar to the way it treats output values, which means it attempts to coerce the value into the field's corresponding data type regardless of the received value. According to items 3f and 3g in section 6.1.2 (http://facebook.github.io/graphql/#sec-Validating-Requests) of Facebook's GraphQL specification query variables should be coerced according to their type's input coercion rules laid out in section 3.1.1 (http://facebook.github.io/graphql/#sec-Scalars). If the value can not be coerced into the correct type according the the input coercion rules for the type a query error should be thrown. This ensures that client provided query variables were of the correct format and will be a valid format and type by the time they are passed into an implementing resolver. This patch fixes the above issue by updating the way query variables are sanitized during the process of parsing the query. It directly follows the rules for scalar input coercion laid out by the specification and throws query errors when a value that cannot be coerced to the correct type is given. Tests for isValidPHPValue will also be updated to ensure that it is doing the correct type checks on Values::isValidPHPValue for the given type and value provided. A new test case will also be added to test Values::getVariableValues and make sure it is also enforcing the scalar input coercion rules and throwing errors for invalid values. --- src/Type/Definition/BooleanType.php | 2 +- src/Type/Definition/FloatType.php | 35 ++--- src/Type/Definition/IDType.php | 11 +- src/Type/Definition/IntType.php | 42 ++---- src/Type/Definition/StringType.php | 11 +- tests/Executor/ExecutorTest.php | 2 +- tests/Executor/ValuesTest.php | 208 ++++++++++++++++++++++++++++ tests/Utils/IsValidPHPValueTest.php | 82 ++++++++--- 8 files changed, 299 insertions(+), 94 deletions(-) create mode 100644 tests/Executor/ValuesTest.php diff --git a/src/Type/Definition/BooleanType.php b/src/Type/Definition/BooleanType.php index 63df5ee..64746a9 100644 --- a/src/Type/Definition/BooleanType.php +++ b/src/Type/Definition/BooleanType.php @@ -34,7 +34,7 @@ class BooleanType extends ScalarType */ public function parseValue($value) { - return !!$value; + return is_bool($value) ? $value : null; } /** diff --git a/src/Type/Definition/FloatType.php b/src/Type/Definition/FloatType.php index 0c8853a..092f258 100644 --- a/src/Type/Definition/FloatType.php +++ b/src/Type/Definition/FloatType.php @@ -32,7 +32,16 @@ values as specified by */ public function serialize($value) { - return $this->coerceFloat($value, false); + if (is_numeric($value) || $value === true || $value === false) { + return (float) $value; + } + + if ($value === '') { + $err = 'Float cannot represent non numeric value: (empty string)'; + } else { + $err = sprintf('Float cannot represent non numeric value: %s', Utils::printSafe($value)); + } + throw new InvariantViolation($err); } /** @@ -41,29 +50,7 @@ values as specified by */ public function parseValue($value) { - return $this->coerceFloat($value, true); - } - - /** - * @param mixed $value - * @param bool $isInput - * @return float|null - */ - private function coerceFloat($value, $isInput) - { - if (is_numeric($value) || $value === true || $value === false) { - return (float) $value; - } - - if ($value === '') { - $err = 'Float cannot represent non numeric value: (empty string)'; - } else { - $err = sprintf( - 'Float cannot represent non numeric value: %s', - $isInput ? Utils::printSafeJson($value) : Utils::printSafe($value) - ); - } - throw ($isInput ? new Error($err) : new InvariantViolation($err)); + return (is_numeric($value) && !is_string($value)) ? (float) $value : null; } /** diff --git a/src/Type/Definition/IDType.php b/src/Type/Definition/IDType.php index 594bea9..49cf7c3 100644 --- a/src/Type/Definition/IDType.php +++ b/src/Type/Definition/IDType.php @@ -55,19 +55,10 @@ When expected as an input type, any string (such as `"4"`) or integer */ public function parseValue($value) { - if ($value === true) { - return 'true'; - } - if ($value === false) { - return 'false'; - } - if ($value === null) { - return 'null'; - } if (!is_scalar($value)) { throw new Error("ID type cannot represent non scalar value: " . Utils::printSafeJson($value)); } - return (string) $value; + return (is_string($value) || is_int($value)) ? (string) $value : null; } /** diff --git a/src/Type/Definition/IntType.php b/src/Type/Definition/IntType.php index 4e797df..b732792 100644 --- a/src/Type/Definition/IntType.php +++ b/src/Type/Definition/IntType.php @@ -38,39 +38,16 @@ values. Int can represent values between -(2^31) and 2^31 - 1. '; */ public function serialize($value) { - return $this->coerceInt($value, false); - } - - /** - * @param mixed $value - * @return int|null - */ - public function parseValue($value) - { - return $this->coerceInt($value, true); - } - - /** - * @param $value - * @param bool $isInput - * @return int|null - */ - private function coerceInt($value, $isInput) - { - $errClass = $isInput ? Error::class : InvariantViolation::class; - if ($value === '') { - throw new $errClass( - 'Int cannot represent non 32-bit signed integer value: (empty string)' - ); + throw new InvariantViolation('Int cannot represent non 32-bit signed integer value: (empty string)'); } if (false === $value || true === $value) { return (int) $value; } if (!is_numeric($value) || $value > self::MAX_INT || $value < self::MIN_INT) { - throw new $errClass(sprintf( + throw new InvariantViolation(sprintf( 'Int cannot represent non 32-bit signed integer value: %s', - $isInput ? Utils::printSafeJson($value) : Utils::printSafe($value) + Utils::printSafe($value) )); } $num = (float) $value; @@ -82,15 +59,24 @@ values. Int can represent values between -(2^31) and 2^31 - 1. '; // Additionally account for scientific notation (i.e. 1e3), because (float)'1e3' is 1000, but (int)'1e3' is 1 $trimmed = floor($num); if ($trimmed !== $num) { - throw new $errClass(sprintf( + throw new InvariantViolation(sprintf( 'Int cannot represent non-integer value: %s', - $isInput ? Utils::printSafeJson($value) : Utils::printSafe($value) + Utils::printSafe($value) )); } } return (int) $value; } + /** + * @param mixed $value + * @return int|null + */ + public function parseValue($value) + { + return (is_int($value) && $value <= self::MAX_INT && $value >= self::MIN_INT) ? $value : null; + } + /** * @param $ast * @return int|null diff --git a/src/Type/Definition/StringType.php b/src/Type/Definition/StringType.php index 46bc942..4cd3815 100644 --- a/src/Type/Definition/StringType.php +++ b/src/Type/Definition/StringType.php @@ -52,19 +52,10 @@ represent free-form human-readable text.'; */ public function parseValue($value) { - if ($value === true) { - return 'true'; - } - if ($value === false) { - return 'false'; - } - if ($value === null) { - return 'null'; - } if (!is_scalar($value)) { throw new Error("String cannot represent non scalar value: " . Utils::printSafe($value)); } - return (string) $value; + return is_string($value) ? $value : null; } /** diff --git a/tests/Executor/ExecutorTest.php b/tests/Executor/ExecutorTest.php index 5e62935..c38045e 100644 --- a/tests/Executor/ExecutorTest.php +++ b/tests/Executor/ExecutorTest.php @@ -248,7 +248,7 @@ class ExecutorTest extends \PHPUnit_Framework_TestCase $rootValue = [ 'root' => 'val' ]; - Executor::execute($schema, $ast, $rootValue, null, [ 'var' => 123 ]); + Executor::execute($schema, $ast, $rootValue, null, [ 'var' => '123' ]); $this->assertEquals([ 'fieldName', diff --git a/tests/Executor/ValuesTest.php b/tests/Executor/ValuesTest.php new file mode 100644 index 0000000..a6da3fe --- /dev/null +++ b/tests/Executor/ValuesTest.php @@ -0,0 +1,208 @@ +expectInputVariablesMatchOutputVariables(['idInput' => '123456789']); + $this->assertEquals( + ['idInput' => '123456789'], + self::runTestCase(['idInput' => 123456789]), + 'Integer ID was not converted to string' + ); + } + + public function testGetBooleanVariableValues() + { + $this->expectInputVariablesMatchOutputVariables(['boolInput' => true]); + $this->expectInputVariablesMatchOutputVariables(['boolInput' => false]); + } + + public function testGetIntVariableValues() + { + $this->expectInputVariablesMatchOutputVariables(['intInput' => -1]); + $this->expectInputVariablesMatchOutputVariables(['intInput' => 0]); + $this->expectInputVariablesMatchOutputVariables(['intInput' => 1]); + + // Test the int size limit + $this->expectInputVariablesMatchOutputVariables(['intInput' => 2147483647]); + $this->expectInputVariablesMatchOutputVariables(['intInput' => -2147483648]); + } + + public function testGetStringVariableValues() + { + $this->expectInputVariablesMatchOutputVariables(['stringInput' => 'meow']); + $this->expectInputVariablesMatchOutputVariables(['stringInput' => '']); + $this->expectInputVariablesMatchOutputVariables(['stringInput' => '1']); + $this->expectInputVariablesMatchOutputVariables(['stringInput' => '0']); + $this->expectInputVariablesMatchOutputVariables(['stringInput' => 'false']); + $this->expectInputVariablesMatchOutputVariables(['stringInput' => '1.2']); + } + + public function testGetFloatVariableValues() + { + $this->expectInputVariablesMatchOutputVariables(['floatInput' => 1.2]); + $this->expectInputVariablesMatchOutputVariables(['floatInput' => 1.0]); + $this->expectInputVariablesMatchOutputVariables(['floatInput' => 1]); + $this->expectInputVariablesMatchOutputVariables(['floatInput' => 0]); + $this->expectInputVariablesMatchOutputVariables(['floatInput' => 1e3]); + } + + public function testBooleanForIDVariableThrowsError() + { + $this->expectGraphQLError(['idInput' => true]); + } + + public function testFloatForIDVariableThrowsError() + { + $this->expectGraphQLError(['idInput' => 1.0]); + } + + public function testStringForBooleanVariableThrowsError() + { + $this->expectGraphQLError(['boolInput' => 'true']); + } + + public function testIntForBooleanVariableThrowsError() + { + $this->expectGraphQLError(['boolInput' => 1]); + } + + public function testFloatForBooleanVariableThrowsError() + { + $this->expectGraphQLError(['boolInput' => 1.0]); + } + + public function testBooleanForIntVariableThrowsError() + { + $this->expectGraphQLError(['intInput' => true]); + } + + public function testStringForIntVariableThrowsError() + { + $this->expectGraphQLError(['intInput' => 'true']); + } + + public function testFloatForIntVariableThrowsError() + { + $this->expectGraphQLError(['intInput' => 1.0]); + } + + public function testPositiveBigIntForIntVariableThrowsError() + { + $this->expectGraphQLError(['intInput' => 2147483648]); + } + + public function testNegativeBigIntForIntVariableThrowsError() + { + $this->expectGraphQLError(['intInput' => -2147483649]); + } + + public function testBooleanForStringVariableThrowsError() + { + $this->expectGraphQLError(['stringInput' => true]); + } + + public function testIntForStringVariableThrowsError() + { + $this->expectGraphQLError(['stringInput' => 1]); + } + + public function testFloatForStringVariableThrowsError() + { + $this->expectGraphQLError(['stringInput' => 1.0]); + } + + public function testBooleanForFloatVariableThrowsError() + { + $this->expectGraphQLError(['floatInput' => true]); + } + + public function testStringForFloatVariableThrowsError() + { + $this->expectGraphQLError(['floatInput' => '1.0']); + } + + // Helpers for running test cases and making assertions + + private function expectInputVariablesMatchOutputVariables($variables) + { + $this->assertEquals( + $variables, + self::runTestCase($variables), + 'Output variables did not match input variables' . PHP_EOL . var_export($variables, true) . PHP_EOL + ); + } + + private function expectGraphQLError($variables) + { + $this->setExpectedException(\GraphQL\Error\Error::class); + self::runTestCase($variables); + } + + private static $schema; + + private static function getSchema() + { + if (!self::$schema) { + self::$schema = new Schema([ + 'query' => new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'test' => [ + 'type' => Type::boolean(), + 'args' => [ + 'idInput' => Type::id(), + 'boolInput' => Type::boolean(), + 'intInput' => Type::int(), + 'stringInput' => Type::string(), + 'floatInput' => Type::float() + ] + ], + ] + ]) + ]); + } + return self::$schema; + } + + private static function getVariableDefinitionNodes() + { + $idInputDefinition = new VariableDefinitionNode([ + 'variable' => new VariableNode(['name' => new NameNode(['value' => 'idInput'])]), + 'type' => new NamedTypeNode(['name' => new NameNode(['value' => 'ID'])]) + ]); + $boolInputDefinition = new VariableDefinitionNode([ + 'variable' => new VariableNode(['name' => new NameNode(['value' => 'boolInput'])]), + 'type' => new NamedTypeNode(['name' => new NameNode(['value' => 'Boolean'])]) + ]); + $intInputDefinition = new VariableDefinitionNode([ + 'variable' => new VariableNode(['name' => new NameNode(['value' => 'intInput'])]), + 'type' => new NamedTypeNode(['name' => new NameNode(['value' => 'Int'])]) + ]); + $stringInputDefintion = new VariableDefinitionNode([ + 'variable' => new VariableNode(['name' => new NameNode(['value' => 'stringInput'])]), + 'type' => new NamedTypeNode(['name' => new NameNode(['value' => 'String'])]) + ]); + $floatInputDefinition = new VariableDefinitionNode([ + 'variable' => new VariableNode(['name' => new NameNode(['value' => 'floatInput'])]), + 'type' => new NamedTypeNode(['name' => new NameNode(['value' => 'Float'])]) + ]); + return [$idInputDefinition, $boolInputDefinition, $intInputDefinition, $stringInputDefintion, $floatInputDefinition]; + } + + private function runTestCase($variables) + { + return Values::getVariableValues(self::getSchema(), self::getVariableDefinitionNodes(), $variables); + } +} \ No newline at end of file diff --git a/tests/Utils/IsValidPHPValueTest.php b/tests/Utils/IsValidPHPValueTest.php index 051f704..937fa4f 100644 --- a/tests/Utils/IsValidPHPValueTest.php +++ b/tests/Utils/IsValidPHPValueTest.php @@ -9,29 +9,43 @@ class IsValidPHPValueTest extends \PHPUnit_Framework_TestCase { public function testValidIntValue() { - // returns no error for int input - $result = Values::isValidPHPValue('1', Type::int()); + // returns no error for positive int value + $result = Values::isValidPHPValue(1, Type::int()); $this->expectNoErrors($result); - // returns no error for negative int input: - $result = Values::isValidPHPValue('-1', Type::int()); + // returns no error for negative int value + $result = Values::isValidPHPValue(-1, Type::int()); $this->expectNoErrors($result); - // returns no error for exponent input: - $result = Values::isValidPHPValue('1e3', Type::int()); - $this->expectNoErrors($result); - $result = Values::isValidPHPValue('0e3', Type::int()); - $this->expectNoErrors($result); - - // returns no error for null value: + // returns no error for null value $result = Values::isValidPHPValue(null, Type::int()); $this->expectNoErrors($result); + // returns a single error for positive int string value + $result = Values::isValidPHPValue('1', Type::int()); + $this->expectErrorResult($result, 1); + + // returns a single error for negative int string value + $result = Values::isValidPHPValue('-1', Type::int()); + $this->expectErrorResult($result, 1); + + // returns errors for exponential int string value + $result = Values::isValidPHPValue('1e3', Type::int()); + $this->expectErrorResult($result, 1); + $result = Values::isValidPHPValue('0e3', Type::int()); + $this->expectErrorResult($result, 1); + // returns a single error for empty value $result = Values::isValidPHPValue('', Type::int()); $this->expectErrorResult($result, 1); - // returns error for float input as int + // returns error for float value + $result = Values::isValidPHPValue(1.5, Type::int()); + $this->expectErrorResult($result, 1); + $result = Values::isValidPHPValue(1e3, Type::int()); + $this->expectErrorResult($result, 1); + + // returns error for float string value $result = Values::isValidPHPValue('1.5', Type::int()); $this->expectErrorResult($result, 1); @@ -46,24 +60,52 @@ class IsValidPHPValueTest extends \PHPUnit_Framework_TestCase public function testValidFloatValue() { - // returns no error for int input - $result = Values::isValidPHPValue('1', Type::float()); + // returns no error for positive float value + $result = Values::isValidPHPValue(1.2, Type::float()); $this->expectNoErrors($result); - // returns no error for exponent input - $result = Values::isValidPHPValue('1e3', Type::float()); - $this->expectNoErrors($result); - $result = Values::isValidPHPValue('0e3', Type::float()); + // returns no error for exponential float value + $result = Values::isValidPHPValue(1e3, Type::float()); $this->expectNoErrors($result); - // returns no error for float input - $result = Values::isValidPHPValue('1.5', Type::float()); + // returns no error for negative float value + $result = Values::isValidPHPValue(-1.2, Type::float()); + $this->expectNoErrors($result); + + // returns no error for a positive int value + $result = Values::isValidPHPValue(1, Type::float()); + $this->expectNoErrors($result); + + // returns no errors for a negative int value + $result = Values::isValidPHPValue(-1, Type::float()); $this->expectNoErrors($result); // returns no error for null value: $result = Values::isValidPHPValue(null, Type::float()); $this->expectNoErrors($result); + // returns error for positive float string value + $result = Values::isValidPHPValue('1.2', Type::float()); + $this->expectErrorResult($result, 1); + + // returns error for negative float string value + $result = Values::isValidPHPValue('-1.2', Type::float()); + $this->expectErrorResult($result, 1); + + // returns error for a positive int string value + $result = Values::isValidPHPValue('1', Type::float()); + $this->expectErrorResult($result, 1); + + // returns errors for a negative int string value + $result = Values::isValidPHPValue('-1', Type::float()); + $this->expectErrorResult($result, 1); + + // returns error for exponent input + $result = Values::isValidPHPValue('1e3', Type::float()); + $this->expectErrorResult($result, 1); + $result = Values::isValidPHPValue('0e3', Type::float()); + $this->expectErrorResult($result, 1); + // returns a single error for empty value $result = Values::isValidPHPValue('', Type::float()); $this->expectErrorResult($result, 1);