From 56ac1061a1f4d2f242db3fdfccd6c82aa6416d26 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Sun, 10 Jun 2018 22:08:04 -0700 Subject: [PATCH 1/2] BREAKING/BUGFIX Strict coercion of scalar types This no longer accepts incoming variable values in a potentially lossy way, mirroring the existing behavior for literals. This fixes an issue with GraphQL.js being not spec compliant. This is breaking since servers which used to accept incorrect variable values will now return errors to clients. Serialization of values is not affected in the same way, since this is not a client-visible behavior. As a bonus, this adds unique serialization and coercion functions for the ID type, allowing to be more restrictive on numeric types and un-stringable object types, while directly supporting valueOf() methods (ala MongoDB). The changes to how the ID type serializes and coerces data could be potentially breaking. Fixes #1324 --- src/execution/__tests__/executor-test.js | 4 +- src/execution/__tests__/variables-test.js | 2 +- src/jsutils/isFinite.js | 20 ++++ src/type/__tests__/serialization-test.js | 68 ++++++++--- src/type/scalars.js | 117 ++++++++++++++----- src/utilities/__tests__/astFromValue-test.js | 7 +- src/utilities/__tests__/coerceValue-test.js | 79 ++++++++----- 7 files changed, 218 insertions(+), 79 deletions(-) create mode 100644 src/jsutils/isFinite.js diff --git a/src/execution/__tests__/executor-test.js b/src/execution/__tests__/executor-test.js index 88f88b2755..da1a155505 100644 --- a/src/execution/__tests__/executor-test.js +++ b/src/execution/__tests__/executor-test.js @@ -279,7 +279,7 @@ describe('Execute: Handles basic execution tasks', () => { const rootValue = { root: 'val' }; - execute(schema, ast, rootValue, null, { var: 123 }); + execute(schema, ast, rootValue, null, { var: 'abc' }); expect(Object.keys(info)).to.deep.equal([ 'fieldName', @@ -304,7 +304,7 @@ describe('Execute: Handles basic execution tasks', () => { expect(info.schema).to.equal(schema); expect(info.rootValue).to.equal(rootValue); expect(info.operation).to.equal(ast.definitions[0]); - expect(info.variableValues).to.deep.equal({ var: '123' }); + expect(info.variableValues).to.deep.equal({ var: 'abc' }); }); it('threads root value context correctly', () => { diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js index 9a72069f58..a7fa5b71fd 100644 --- a/src/execution/__tests__/variables-test.js +++ b/src/execution/__tests__/variables-test.js @@ -694,7 +694,7 @@ describe('Execute: Handles inputs', () => { { message: 'Variable "$value" got invalid value [1, 2, 3]; Expected type ' + - 'String; String cannot represent an array value: [1, 2, 3]', + 'String; String cannot represent a non string value: [1, 2, 3]', locations: [{ line: 2, column: 16 }], }, ], diff --git a/src/jsutils/isFinite.js b/src/jsutils/isFinite.js new file mode 100644 index 0000000000..a874a2d846 --- /dev/null +++ b/src/jsutils/isFinite.js @@ -0,0 +1,20 @@ +/** + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow strict + */ + +declare function isFinite(value: mixed): boolean %checks(typeof value === + 'number'); + +/* eslint-disable no-redeclare */ +// $FlowFixMe workaround for: https://github.com/facebook/flow/issues/4441 +const isFinite = + Number.isFinite || + function(value) { + return typeof value === 'number' && isFinite(value); + }; +export default isFinite; diff --git a/src/type/__tests__/serialization-test.js b/src/type/__tests__/serialization-test.js index d6bf307a9d..86895b13fa 100644 --- a/src/type/__tests__/serialization-test.js +++ b/src/type/__tests__/serialization-test.js @@ -60,7 +60,7 @@ describe('Type System: Scalar coercion', () => { ); // Doesn't represent number expect(() => GraphQLInt.serialize('')).to.throw( - 'Int cannot represent non-integer value: (empty string)', + 'Int cannot represent non-integer value: ""', ); expect(() => GraphQLInt.serialize(NaN)).to.throw( 'Int cannot represent non-integer value: NaN', @@ -95,26 +95,24 @@ describe('Type System: Scalar coercion', () => { 'Float cannot represent non numeric value: "one"', ); expect(() => GraphQLFloat.serialize('')).to.throw( - 'Float cannot represent non numeric value: (empty string)', + 'Float cannot represent non numeric value: ""', ); expect(() => GraphQLFloat.serialize([5])).to.throw( 'Float cannot represent an array value: [5]', ); }); - for (const scalar of [GraphQLString, GraphQLID]) { - it(`serializes output as ${scalar}`, () => { - expect(scalar.serialize('string')).to.equal('string'); - expect(scalar.serialize(1)).to.equal('1'); - expect(scalar.serialize(-1.1)).to.equal('-1.1'); - expect(scalar.serialize(true)).to.equal('true'); - expect(scalar.serialize(false)).to.equal('false'); + it(`serializes output as String`, () => { + expect(GraphQLString.serialize('string')).to.equal('string'); + expect(GraphQLString.serialize(1)).to.equal('1'); + expect(GraphQLString.serialize(-1.1)).to.equal('-1.1'); + expect(GraphQLString.serialize(true)).to.equal('true'); + expect(GraphQLString.serialize(false)).to.equal('false'); - expect(() => scalar.serialize([1])).to.throw( - 'String cannot represent an array value: [1]', - ); - }); - } + expect(() => GraphQLString.serialize([1])).to.throw( + 'String cannot represent an array value: [1]', + ); + }); it('serializes output as Boolean', () => { expect(GraphQLBoolean.serialize('string')).to.equal(true); @@ -129,4 +127,46 @@ describe('Type System: Scalar coercion', () => { 'Boolean cannot represent an array value: [false]', ); }); + + it('serializes output as ID', () => { + expect(GraphQLID.serialize('string')).to.equal('string'); + expect(GraphQLID.serialize('false')).to.equal('false'); + expect(GraphQLID.serialize('')).to.equal(''); + expect(GraphQLID.serialize(123)).to.equal('123'); + expect(GraphQLID.serialize(0)).to.equal('0'); + + const objValue = { + _id: 123, + valueOf() { + return this._id; + }, + }; + expect(GraphQLID.serialize(objValue)).to.equal('123'); + + const badObjValue = { + _id: false, + valueOf() { + return this._id; + }, + }; + expect(() => GraphQLID.serialize(badObjValue)).to.throw( + 'ID cannot represent value: {_id: false, valueOf: [function valueOf]}', + ); + + expect(() => GraphQLID.serialize(true)).to.throw( + 'ID cannot represent value: true', + ); + + expect(() => GraphQLID.serialize(-1.1)).to.throw( + 'ID cannot represent value: -1.1', + ); + + expect(() => GraphQLID.serialize({})).to.throw( + 'ID cannot represent value: {}', + ); + + expect(() => GraphQLID.serialize(['abc'])).to.throw( + 'ID cannot represent value: ["abc"]', + ); + }); }); diff --git a/src/type/scalars.js b/src/type/scalars.js index 72400b2591..db721c04f2 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -8,6 +8,7 @@ */ import inspect from '../jsutils/inspect'; +import isFinite from '../jsutils/isFinite'; import isInteger from '../jsutils/isInteger'; import { GraphQLScalarType, isNamedType } from './definition'; import { Kind } from '../language/kinds'; @@ -20,30 +21,38 @@ import { Kind } from '../language/kinds'; const MAX_INT = 2147483647; const MIN_INT = -2147483648; -function coerceInt(value: mixed): number { +function serializeInt(value: mixed): number { if (Array.isArray(value)) { throw new TypeError( - `Int cannot represent an array value: [${String(value)}]`, + `Int cannot represent an array value: ${inspect(value)}`, ); } - if (value === '') { + const num = Number(value); + if (value === '' || !isInteger(num)) { throw new TypeError( - 'Int cannot represent non-integer value: (empty string)', + `Int cannot represent non-integer value: ${inspect(value)}`, ); } - const num = Number(value); - if (!isInteger(num)) { + if (num > MAX_INT || num < MIN_INT) { throw new TypeError( - 'Int cannot represent non-integer value: ' + inspect(value), + `Int cannot represent non 32-bit signed integer value: ${inspect(value)}`, ); } + return num; +} - if (num > MAX_INT || num < MIN_INT) { +function coerceInt(value: mixed): number { + if (!isInteger(value)) { throw new TypeError( - 'Int cannot represent non 32-bit signed integer value: ' + inspect(value), + `Int cannot represent non-integer value: ${inspect(value)}`, ); } - return num; + if (value > MAX_INT || value < MIN_INT) { + throw new TypeError( + `Int cannot represent non 32-bit signed integer value: ${inspect(value)}`, + ); + } + return value; } export const GraphQLInt = new GraphQLScalarType({ @@ -51,7 +60,7 @@ export const GraphQLInt = new GraphQLScalarType({ description: 'The `Int` scalar type represents non-fractional signed whole numeric ' + 'values. Int can represent values between -(2^31) and 2^31 - 1. ', - serialize: coerceInt, + serialize: serializeInt, parseValue: coerceInt, parseLiteral(ast) { if (ast.kind === Kind.INT) { @@ -64,24 +73,28 @@ export const GraphQLInt = new GraphQLScalarType({ }, }); -function coerceFloat(value: mixed): number { +function serializeFloat(value: mixed): number { if (Array.isArray(value)) { throw new TypeError( - `Float cannot represent an array value: [${String(value)}]`, + `Float cannot represent an array value: ${inspect(value)}`, ); } - if (value === '') { + const num = Number(value); + if (value === '' || !isFinite(num)) { throw new TypeError( - 'Float cannot represent non numeric value: (empty string)', + `Float cannot represent non numeric value: ${inspect(value)}`, ); } - const num = Number(value); - if (isFinite(num)) { - return num; + return num; +} + +function coerceFloat(value: mixed): number { + if (!isFinite(value)) { + throw new TypeError( + `Float cannot represent non numeric value: ${inspect(value)}`, + ); } - throw new TypeError( - 'Float cannot represent non numeric value: ' + inspect(value), - ); + return value; } export const GraphQLFloat = new GraphQLScalarType({ @@ -90,7 +103,7 @@ export const GraphQLFloat = new GraphQLScalarType({ 'The `Float` scalar type represents signed double-precision fractional ' + 'values as specified by ' + '[IEEE 754](http://en.wikipedia.org/wiki/IEEE_floating_point). ', - serialize: coerceFloat, + serialize: serializeFloat, parseValue: coerceFloat, parseLiteral(ast) { return ast.kind === Kind.FLOAT || ast.kind === Kind.INT @@ -99,7 +112,7 @@ export const GraphQLFloat = new GraphQLScalarType({ }, }); -function coerceString(value: mixed): string { +function serializeString(value: mixed): string { if (Array.isArray(value)) { throw new TypeError( `String cannot represent an array value: ${inspect(value)}`, @@ -108,38 +121,84 @@ function coerceString(value: mixed): string { return String(value); } +function coerceString(value: mixed): string { + if (typeof value !== 'string') { + throw new TypeError( + `String cannot represent a non string value: ${inspect(value)}`, + ); + } + return value; +} + export const GraphQLString = new GraphQLScalarType({ name: 'String', description: 'The `String` scalar type represents textual data, represented as UTF-8 ' + 'character sequences. The String type is most often used by GraphQL to ' + 'represent free-form human-readable text.', - serialize: coerceString, + serialize: serializeString, parseValue: coerceString, parseLiteral(ast) { return ast.kind === Kind.STRING ? ast.value : undefined; }, }); -function coerceBoolean(value: mixed): boolean { +function serializeBoolean(value: mixed): boolean { if (Array.isArray(value)) { throw new TypeError( - `Boolean cannot represent an array value: [${String(value)}]`, + `Boolean cannot represent an array value: ${inspect(value)}`, ); } return Boolean(value); } +function coerceBoolean(value: mixed): boolean { + if (typeof value !== 'boolean') { + throw new TypeError( + `Boolean cannot represent a non boolean value: ${inspect(value)}`, + ); + } + return value; +} + export const GraphQLBoolean = new GraphQLScalarType({ name: 'Boolean', description: 'The `Boolean` scalar type represents `true` or `false`.', - serialize: coerceBoolean, + serialize: serializeBoolean, parseValue: coerceBoolean, parseLiteral(ast) { return ast.kind === Kind.BOOLEAN ? ast.value : undefined; }, }); +function serializeID(value: mixed): string { + // Support serializing objects with custom valueOf() functions - a common way + // to represent an object identifier (ex. MongoDB). + const result = + value && + typeof value.valueOf === 'function' && + value.valueOf !== Object.prototype.valueOf + ? value.valueOf() + : value; + if ( + typeof result !== 'string' && + (typeof result !== 'number' || !isInteger(result)) + ) { + throw new TypeError(`ID cannot represent value: ${inspect(value)}`); + } + return String(result); +} + +function coerceID(value: mixed): string { + if ( + typeof value !== 'string' && + (typeof value !== 'number' || !isInteger(value)) + ) { + throw new TypeError(`ID cannot represent value: ${inspect(value)}`); + } + return String(value); +} + export const GraphQLID = new GraphQLScalarType({ name: 'ID', description: @@ -148,8 +207,8 @@ export const GraphQLID = new GraphQLScalarType({ 'response as a String; however, it is not intended to be human-readable. ' + 'When expected as an input type, any string (such as `"4"`) or integer ' + '(such as `4`) input value will be accepted as an ID.', - serialize: coerceString, - parseValue: coerceString, + serialize: serializeID, + parseValue: coerceID, parseLiteral(ast) { return ast.kind === Kind.STRING || ast.kind === Kind.INT ? ast.value diff --git a/src/utilities/__tests__/astFromValue-test.js b/src/utilities/__tests__/astFromValue-test.js index 93d869a6e1..647a6eb552 100644 --- a/src/utilities/__tests__/astFromValue-test.js +++ b/src/utilities/__tests__/astFromValue-test.js @@ -183,10 +183,9 @@ describe('astFromValue', () => { value: '01', }); - expect(astFromValue(false, GraphQLID)).to.deep.equal({ - kind: 'StringValue', - value: 'false', - }); + expect(() => astFromValue(false, GraphQLID)).to.throw( + 'ID cannot represent value: false', + ); expect(astFromValue(null, GraphQLID)).to.deep.equal({ kind: 'NullValue' }); diff --git a/src/utilities/__tests__/coerceValue-test.js b/src/utilities/__tests__/coerceValue-test.js index 7adb1155da..e3e65df148 100644 --- a/src/utilities/__tests__/coerceValue-test.js +++ b/src/utilities/__tests__/coerceValue-test.js @@ -30,34 +30,48 @@ function expectErrors(result) { } describe('coerceValue', () => { - for (const scalar of [GraphQLString, GraphQLID]) { - describe(`for GraphQL${scalar}`, () => { - it('returns error for array input as string', () => { - const result = coerceValue([1, 2, 3], scalar); - expectErrors(result).to.deep.equal([ - `Expected type ${scalar}; String cannot represent an array value: [1, 2, 3]`, - ]); - }); + describe(`for GraphQLString`, () => { + it('returns error for array input as string', () => { + const result = coerceValue([1, 2, 3], GraphQLString); + expectErrors(result).to.deep.equal([ + `Expected type String; String cannot represent a non string value: [1, 2, 3]`, + ]); }); - } + }); + + describe(`for GraphQLID`, () => { + it('returns error for array input as string', () => { + const result = coerceValue([1, 2, 3], GraphQLID); + expectErrors(result).to.deep.equal([ + `Expected type ID; ID cannot represent value: [1, 2, 3]`, + ]); + }); + }); describe('for GraphQLInt', () => { - it('returns no error for int input', () => { - const result = coerceValue('1', GraphQLInt); + it('returns value for integer', () => { + const result = coerceValue(1, GraphQLInt); expectValue(result).to.equal(1); }); - it('returns no error for negative int input', () => { - const result = coerceValue('-1', GraphQLInt); + it('returns error for numeric looking string', () => { + const result = coerceValue('1', GraphQLInt); + expectErrors(result).to.deep.equal([ + `Expected type Int; Int cannot represent non-integer value: "1"`, + ]); + }); + + it('returns value for negative int input', () => { + const result = coerceValue(-1, GraphQLInt); expectValue(result).to.equal(-1); }); - it('returns no error for exponent input', () => { - const result = coerceValue('1e3', GraphQLInt); + it('returns value for exponent input', () => { + const result = coerceValue(1e3, GraphQLInt); expectValue(result).to.equal(1000); }); - it('returns a single error for empty value', () => { + it('returns null for null value', () => { const result = coerceValue(null, GraphQLInt); expectValue(result).to.equal(null); }); @@ -65,7 +79,7 @@ describe('coerceValue', () => { it('returns a single error for empty string as value', () => { const result = coerceValue('', GraphQLInt); expectErrors(result).to.deep.equal([ - 'Expected type Int; Int cannot represent non-integer value: (empty string)', + 'Expected type Int; Int cannot represent non-integer value: ""', ]); }); @@ -77,9 +91,9 @@ describe('coerceValue', () => { }); it('returns a single error for float input as int', () => { - const result = coerceValue('1.5', GraphQLInt); + const result = coerceValue(1.5, GraphQLInt); expectErrors(result).to.deep.equal([ - 'Expected type Int; Int cannot represent non-integer value: "1.5"', + 'Expected type Int; Int cannot represent non-integer value: 1.5', ]); }); @@ -104,7 +118,7 @@ describe('coerceValue', () => { ]); }); - it('returns a single error for char input', () => { + it('returns a single error for string input', () => { const result = coerceValue('meow', GraphQLInt); expectErrors(result).to.deep.equal([ 'Expected type Int; Int cannot represent non-integer value: "meow"', @@ -113,22 +127,29 @@ describe('coerceValue', () => { }); describe('for GraphQLFloat', () => { - it('returns no error for int input', () => { - const result = coerceValue('1', GraphQLFloat); + it('returns value for integer', () => { + const result = coerceValue(1, GraphQLFloat); expectValue(result).to.equal(1); }); - it('returns no error for exponent input', () => { - const result = coerceValue('1e3', GraphQLFloat); + it('returns value for decimal', () => { + const result = coerceValue(1.1, GraphQLFloat); + expectValue(result).to.equal(1.1); + }); + + it('returns value for exponent input', () => { + const result = coerceValue(1e3, GraphQLFloat); expectValue(result).to.equal(1000); }); - it('returns no error for float input', () => { - const result = coerceValue('1.5', GraphQLFloat); - expectValue(result).to.equal(1.5); + it('returns error for numeric looking string', () => { + const result = coerceValue('1', GraphQLFloat); + expectErrors(result).to.deep.equal([ + 'Expected type Float; Float cannot represent non numeric value: "1"', + ]); }); - it('returns a single error for empty value', () => { + it('returns null for null value', () => { const result = coerceValue(null, GraphQLFloat); expectValue(result).to.equal(null); }); @@ -136,7 +157,7 @@ describe('coerceValue', () => { it('returns a single error for empty string input', () => { const result = coerceValue('', GraphQLFloat); expectErrors(result).to.deep.equal([ - 'Expected type Float; Float cannot represent non numeric value: (empty string)', + 'Expected type Float; Float cannot represent non numeric value: ""', ]); }); From 934d46102369d8d90e0141cad1bbb0680de16aac Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Mon, 11 Jun 2018 21:03:05 -0700 Subject: [PATCH 2/2] Updates from review. Simplified ID serialization and added similar logic to string serialization --- src/type/__tests__/serialization-test.js | 16 ++++++++++++++- src/type/scalars.js | 25 ++++++++++++++---------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/type/__tests__/serialization-test.js b/src/type/__tests__/serialization-test.js index 86895b13fa..0944303d0c 100644 --- a/src/type/__tests__/serialization-test.js +++ b/src/type/__tests__/serialization-test.js @@ -110,7 +110,21 @@ describe('Type System: Scalar coercion', () => { expect(GraphQLString.serialize(false)).to.equal('false'); expect(() => GraphQLString.serialize([1])).to.throw( - 'String cannot represent an array value: [1]', + 'String cannot represent value: [1]', + ); + + const badObjValue = {}; + expect(() => GraphQLString.serialize(badObjValue)).to.throw( + 'String cannot represent value: {}', + ); + + const stringableObjValue = { + valueOf() { + return 'something useful'; + }, + }; + expect(GraphQLString.serialize(stringableObjValue)).to.equal( + 'something useful', ); }); diff --git a/src/type/scalars.js b/src/type/scalars.js index db721c04f2..9962e52940 100644 --- a/src/type/scalars.js +++ b/src/type/scalars.js @@ -113,12 +113,21 @@ export const GraphQLFloat = new GraphQLScalarType({ }); function serializeString(value: mixed): string { - if (Array.isArray(value)) { - throw new TypeError( - `String cannot represent an array value: ${inspect(value)}`, - ); + // Support serializing objects with custom valueOf() functions - a common way + // to represent an complex value which can be represented as a string + // (ex: MongoDB id objects). + const result = + value && typeof value.valueOf === 'function' ? value.valueOf() : value; + // Serialize string, number, and boolean values to a string, but do not + // attempt to coerce object, function, symbol, or other types as strings. + if ( + typeof result !== 'string' && + typeof result !== 'number' && + typeof result !== 'boolean' + ) { + throw new TypeError(`String cannot represent value: ${inspect(result)}`); } - return String(value); + return String(result); } function coerceString(value: mixed): string { @@ -175,11 +184,7 @@ function serializeID(value: mixed): string { // Support serializing objects with custom valueOf() functions - a common way // to represent an object identifier (ex. MongoDB). const result = - value && - typeof value.valueOf === 'function' && - value.valueOf !== Object.prototype.valueOf - ? value.valueOf() - : value; + value && typeof value.valueOf === 'function' ? value.valueOf() : value; if ( typeof result !== 'string' && (typeof result !== 'number' || !isInteger(result))