From 62f53564f1a5d648e8ed67bbdf5c50db2b12f50f Mon Sep 17 00:00:00 2001 From: SoyYoRafa <2441438+SoyYoRafa@users.noreply.github.com> Date: Sat, 13 Jul 2019 20:14:40 -0500 Subject: [PATCH] Fixes for issue 1753. Adds ability to limit errors in getVariableValues() and coerceValue(). Errors are statically capped at 50 --- src/execution/__tests__/variables-test.js | 119 ++++++++++++++++++++ src/execution/values.js | 31 ++++- src/utilities/__tests__/coerceValue-test.js | 84 ++++++++++++++ src/utilities/coerceValue.js | 32 +++++- 4 files changed, 258 insertions(+), 8 deletions(-) diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js index 33fafeeb5d..9c90dff376 100644 --- a/src/execution/__tests__/variables-test.js +++ b/src/execution/__tests__/variables-test.js @@ -80,6 +80,22 @@ function fieldWithInputArg(inputArg) { }; } +function fieldsWithOneHundredInputArg(inputArg) { + const args = {}; + for (let index = 0; index < 100; ++index) { + args[`input${index}`] = inputArg; + } + return { + type: GraphQLString, + args, + resolve(_, rargs) { + if ('input0' in rargs) { + return inspect(rargs.input0); + } + }, + }; +} + const TestType = new GraphQLObjectType({ name: 'TestType', fields: { @@ -104,6 +120,9 @@ const TestType = new GraphQLObjectType({ type: TestNestedInputObject, defaultValue: 'Hello World', }), + fieldWithOneHundredInputs: fieldsWithOneHundredInputArg({ + type: GraphQLNonNull(GraphQLString), + }), list: fieldWithInputArg({ type: GraphQLList(GraphQLString) }), nnList: fieldWithInputArg({ type: GraphQLNonNull(GraphQLList(GraphQLString)), @@ -715,6 +734,43 @@ describe('Execute: Handles inputs', () => { ], }); }); + + it('limits errors when there are too many null values provided for non-nullable inputs', () => { + let queryFields = ''; + let fieldArguments = ''; + const valueObject = {}; + const errors = []; + for (let index = 0; index < 100; ++index) { + queryFields += `$input${index}: String!`; + fieldArguments = `input${index}: $input${index}`; + if (index !== 99) { + queryFields += ','; + fieldArguments += ','; + } + valueObject[`input${index}`] = null; + if (index < 50) { + const column = index < 10 ? 16 + index * 17 : 16 + index * 18 - 10; + errors.push({ + message: `Variable "$input${index}" of non-null type "String!" must not be null.`, + locations: [{ line: 2, column }], + }); + } + } + errors.push({ + message: + 'Too many errors processing variables, error limit reached. Execution aborted.', + }); + const doc = ` + query (${queryFields}) { + fieldWithOneHundredInputs(${fieldArguments}) + } + `; + const result = executeQuery(doc, valueObject); + + expect(result).to.deep.equal({ + errors, + }); + }); }); describe('Handles lists and nullability', () => { @@ -882,6 +938,32 @@ describe('Execute: Handles inputs', () => { }); }); + it('limits errors when there are more than 50 errors of does not allow non-null lists of non-nulls to contain null.', () => { + const doc = ` + query ($input: [String!]!) { + nnListNN(input: $input) + } + `; + const largeList = new Array(100).fill(null); + const result = executeQuery(doc, { input: largeList }); + + const expectedErrors = []; + for (let idx = 0; idx < 50; ++idx) { + expectedErrors.push({ + message: `Variable "$input" got invalid value [null, null, null, null, null, null, null, null, null, null, ... 90 more items]; Expected non-nullable type String! not to be null at value[${idx}].`, + locations: [{ line: 2, column: 16 }], + }); + } + expectedErrors.push({ + message: + 'Too many errors processing variables, error limit reached. Execution aborted.', + }); + + expect(result).to.deep.equal({ + errors: expectedErrors, + }); + }); + it('does not allow invalid types to be used as values', () => { const doc = ` query ($input: TestType!) { @@ -919,6 +1001,43 @@ describe('Execute: Handles inputs', () => { ], }); }); + + it('limits errors when there are too many unknown types to be used as values', () => { + let queryFields = ''; + let fieldArguments = ''; + const valueObject = {}; + const errors = []; + for (let index = 0; index < 100; ++index) { + queryFields += `$input${index}: UnknownType!`; + fieldArguments = `input${index}: $input${index}`; + if (index !== 99) { + queryFields += ','; + fieldArguments += ','; + } + valueObject[`input${index}`] = 'whoknows'; + if (index < 50) { + const column = index < 10 ? 25 + index * 22 : 25 + index * 23 - 9; + errors.push({ + message: `Variable "$input${index}" expected value of type "UnknownType!" which cannot be used as an input type.`, + locations: [{ line: 2, column }], + }); + } + } + errors.push({ + message: + 'Too many errors processing variables, error limit reached. Execution aborted.', + }); + const doc = ` + query (${queryFields}) { + fieldWithOneHundredInputs(${fieldArguments}) + } + `; + const result = executeQuery(doc, valueObject); + + expect(result).to.deep.equal({ + errors, + }); + }); }); describe('Execute: Uses argument default values', () => { diff --git a/src/execution/values.js b/src/execution/values.js index b941374fbf..5ac02c480e 100644 --- a/src/execution/values.js +++ b/src/execution/values.js @@ -45,6 +45,8 @@ export function getVariableValues( ): CoercedVariableValues { const errors = []; const coercedValues = {}; + const maxErrors = 50; + let errorLimitReached = false; for (let i = 0; i < varDefNodes.length; i++) { const varDefNode = varDefNodes[i]; const varName = varDefNode.variable.name.value; @@ -61,6 +63,10 @@ export function getVariableValues( varDefNode.type, ), ); + if (errors.length === maxErrors) { + errorLimitReached = true; + break; + } } else { const hasValue = hasOwnProperty(inputs, varName); const value = hasValue ? inputs[varName] : undefined; @@ -81,6 +87,10 @@ export function getVariableValues( varDefNode, ), ); + if (errors.length === maxErrors) { + errorLimitReached = true; + break; + } } else if (hasValue) { if (value === null) { // If the explicit value `null` was provided, an entry in the coerced @@ -92,12 +102,21 @@ export function getVariableValues( const coerced = coerceValue(value, varType, varDefNode); const coercionErrors = coerced.errors; if (coercionErrors) { - for (const error of coercionErrors) { + const reachesErrorLimit = + coercionErrors.length + errors.length >= maxErrors; + const publishedErrors = reachesErrorLimit + ? coercionErrors + : coercionErrors.slice(0, maxErrors - errors.length); + for (const error of publishedErrors) { error.message = `Variable "$${varName}" got invalid value ${inspect(value)}; ` + error.message; } - errors.push(...coercionErrors); + errors.push(...publishedErrors); + if (reachesErrorLimit || coerced.errorLimitReached) { + errorLimitReached = true; + break; + } } else { coercedValues[varName] = coerced.value; } @@ -105,6 +124,14 @@ export function getVariableValues( } } } + if (errorLimitReached) { + errors.push( + new GraphQLError( + 'Too many errors processing variables, error limit reached. Execution aborted.', + ), + ); + } + return errors.length === 0 ? { errors: undefined, coerced: coercedValues } : { errors, coerced: undefined }; diff --git a/src/utilities/__tests__/coerceValue-test.js b/src/utilities/__tests__/coerceValue-test.js index ae53957ebb..9a30895215 100644 --- a/src/utilities/__tests__/coerceValue-test.js +++ b/src/utilities/__tests__/coerceValue-test.js @@ -16,11 +16,13 @@ import { function expectValue(result) { expect(result.errors).to.equal(undefined); + expect(result.errorLimitReached).to.equal(undefined); return expect(result.value); } function expectErrors(result) { expect(result.value).to.equal(undefined); + expect(result.errorLimitReached).to.not.equal(undefined); const messages = result.errors && result.errors.map(error => error.message); return expect(messages); } @@ -255,6 +257,32 @@ describe('coerceValue', () => { ]); }); + it('limits errors for too many invalid fields', () => { + const TestInputBigObjectConfig = { + name: 'TestInputBigObject', + fields: {}, + }; + const valueObject = {}; + const errors = []; + for (let index = 0; index < 100; ++index) { + TestInputBigObjectConfig.fields[`field${index}`] = { + type: GraphQLNonNull(GraphQLInt), + }; + valueObject[`field${index}`] = 'abc'; + if (index < 50) { + errors.push( + `Expected type Int at value.field${index}. Int cannot represent non-integer value: "abc"`, + ); + } + } + const TestInputBigObject = new GraphQLInputObjectType( + TestInputBigObjectConfig, + ); + const result = coerceValue(valueObject, TestInputBigObject); + expectErrors(result).to.deep.equal(errors); + expect(result.errorLimitReached).to.equal(true); + }); + it('returns error for a missing required field', () => { const result = coerceValue({ bar: 123 }, TestInputObject); expectErrors(result).to.deep.equal([ @@ -262,6 +290,30 @@ describe('coerceValue', () => { ]); }); + it('limits errors for too many missing required fields', () => { + const TestInputBigObjectConfig = { + name: 'TestInputBigObject', + fields: {}, + }; + const errors = []; + for (let index = 0; index < 100; ++index) { + TestInputBigObjectConfig.fields[`field${index}`] = { + type: GraphQLNonNull(GraphQLInt), + }; + if (index < 50) { + errors.push( + `Field value.field${index} of required type Int! was not provided.`, + ); + } + } + const TestInputBigObject = new GraphQLInputObjectType( + TestInputBigObjectConfig, + ); + const result = coerceValue({}, TestInputBigObject); + expectErrors(result).to.deep.equal(errors); + expect(result.errorLimitReached).to.equal(true); + }); + it('returns error for an unknown field', () => { const result = coerceValue( { foo: 123, unknownField: 123 }, @@ -272,6 +324,22 @@ describe('coerceValue', () => { ]); }); + it('limits errors for too many unkown fields', () => { + const valueObject = { foo: 123 }; + const errors = []; + for (let index = 0; index < 100; ++index) { + valueObject[`field${index}`] = 'string'; + if (index < 50) { + errors.push( + `Field "field${index}" is not defined by type TestInputObject.`, + ); + } + } + const result = coerceValue(valueObject, TestInputObject); + expectErrors(result).to.deep.equal(errors); + expect(result.errorLimitReached).to.equal(true); + }); + it('returns error for a misspelled field', () => { const result = coerceValue({ foo: 123, bart: 123 }, TestInputObject); expectErrors(result).to.deep.equal([ @@ -334,5 +402,21 @@ describe('coerceValue', () => { const result = coerceValue([42, [null], null], TestNestedList); expectValue(result).to.deep.equal([[42], [null], null]); }); + + it('returns an error array limited to 50 errors and limit reached flag is true', () => { + const value = []; + const errors = []; + for (let index = 0; index < 100; ++index) { + value.push(['string']); + if (index < 50) { + errors.push( + `Expected type Int at value[${index}][0]. Int cannot represent non-integer value: "string"`, + ); + } + } + const result = coerceValue(value, TestNestedList); + expectErrors(result).to.deep.equal(errors); + expect(result.errorLimitReached).to.equal(true); + }); }); }); diff --git a/src/utilities/coerceValue.js b/src/utilities/coerceValue.js index b425147319..d34103b382 100644 --- a/src/utilities/coerceValue.js +++ b/src/utilities/coerceValue.js @@ -1,6 +1,6 @@ // @flow strict -import { forEach, isCollection } from 'iterall'; +import { isCollection } from 'iterall'; import objectValues from '../polyfills/objectValues'; import inspect from '../jsutils/inspect'; import isInvalid from '../jsutils/isInvalid'; @@ -19,6 +19,7 @@ import { } from '../type/definition'; type CoercedValue = {| + +errorLimitReached: boolean | void, +errors: $ReadOnlyArray | void, +value: mixed, |}; @@ -38,6 +39,7 @@ export function coerceValue( blameNode?: ASTNode, path?: Path, ): CoercedValue { + const maxErrors = 50; // A value must be provided if the type is non-null. if (isNonNullType(type)) { if (value == null) { @@ -108,7 +110,9 @@ export function coerceValue( if (isCollection(value)) { let errors; const coercedValue = []; - forEach((value: any), (itemValue, index) => { + const listValue: $ReadOnlyArray = (value: any); + for (let index = 0; index < listValue.length; ++index) { + const itemValue = listValue[index]; const coercedItem = coerceValue( itemValue, itemType, @@ -117,10 +121,13 @@ export function coerceValue( ); if (coercedItem.errors) { errors = add(errors, coercedItem.errors); + if (errors.length >= maxErrors) { + return ofErrors(errors.slice(0, maxErrors), true); + } } else if (!errors) { coercedValue.push(coercedItem.value); } - }); + } return errors ? ofErrors(errors) : ofValue(coercedValue); } // Lists accept a non-list value as a list of one. @@ -157,6 +164,9 @@ export function coerceValue( blameNode, ), ); + if (errors.length >= maxErrors) { + return ofErrors(errors.slice(0, maxErrors), true); + } } } else { const coercedField = coerceValue( @@ -167,6 +177,9 @@ export function coerceValue( ); if (coercedField.errors) { errors = add(errors, coercedField.errors); + if (errors.length >= maxErrors) { + return ofErrors(errors.slice(0, maxErrors), true); + } } else if (!errors) { coercedValue[field.name] = coercedField.value; } @@ -186,6 +199,9 @@ export function coerceValue( didYouMean(suggestions), ), ); + if (errors.length >= maxErrors) { + return ofErrors(errors.slice(0, maxErrors), true); + } } } @@ -198,11 +214,15 @@ export function coerceValue( } function ofValue(value) { - return { errors: undefined, value }; + return { errorLimitReached: undefined, errors: undefined, value }; } -function ofErrors(errors) { - return { errors, value: undefined }; +function ofErrors(errors, errorLimitReached) { + return { + errorLimitReached: errorLimitReached == null ? false : errorLimitReached, + errors, + value: undefined, + }; } function add(errors, moreErrors) {