Skip to content

Commit 62f5356

Browse files
SoyYoRafaSoyYoRafa
committed
Fixes for issue 1753. Adds ability to limit errors in getVariableValues() and coerceValue(). Errors are statically capped at 50
1 parent 49d86bb commit 62f5356

File tree

4 files changed

+258
-8
lines changed

4 files changed

+258
-8
lines changed

‎src/execution/__tests__/variables-test.js

+119
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,22 @@ function fieldWithInputArg(inputArg) {
8080
};
8181
}
8282

83+
function fieldsWithOneHundredInputArg(inputArg) {
84+
const args = {};
85+
for (let index = 0; index < 100; ++index) {
86+
args[`input${index}`] = inputArg;
87+
}
88+
return {
89+
type: GraphQLString,
90+
args,
91+
resolve(_, rargs) {
92+
if ('input0' in rargs) {
93+
return inspect(rargs.input0);
94+
}
95+
},
96+
};
97+
}
98+
8399
const TestType = new GraphQLObjectType({
84100
name: 'TestType',
85101
fields: {
@@ -104,6 +120,9 @@ const TestType = new GraphQLObjectType({
104120
type: TestNestedInputObject,
105121
defaultValue: 'Hello World',
106122
}),
123+
fieldWithOneHundredInputs: fieldsWithOneHundredInputArg({
124+
type: GraphQLNonNull(GraphQLString),
125+
}),
107126
list: fieldWithInputArg({ type: GraphQLList(GraphQLString) }),
108127
nnList: fieldWithInputArg({
109128
type: GraphQLNonNull(GraphQLList(GraphQLString)),
@@ -715,6 +734,43 @@ describe('Execute: Handles inputs', () => {
715734
],
716735
});
717736
});
737+
738+
it('limits errors when there are too many null values provided for non-nullable inputs', () => {
739+
let queryFields = '';
740+
let fieldArguments = '';
741+
const valueObject = {};
742+
const errors = [];
743+
for (let index = 0; index < 100; ++index) {
744+
queryFields += `$input${index}: String!`;
745+
fieldArguments = `input${index}: $input${index}`;
746+
if (index !== 99) {
747+
queryFields += ',';
748+
fieldArguments += ',';
749+
}
750+
valueObject[`input${index}`] = null;
751+
if (index < 50) {
752+
const column = index < 10 ? 16 + index * 17 : 16 + index * 18 - 10;
753+
errors.push({
754+
message: `Variable "$input${index}" of non-null type "String!" must not be null.`,
755+
locations: [{ line: 2, column }],
756+
});
757+
}
758+
}
759+
errors.push({
760+
message:
761+
'Too many errors processing variables, error limit reached. Execution aborted.',
762+
});
763+
const doc = `
764+
query (${queryFields}) {
765+
fieldWithOneHundredInputs(${fieldArguments})
766+
}
767+
`;
768+
const result = executeQuery(doc, valueObject);
769+
770+
expect(result).to.deep.equal({
771+
errors,
772+
});
773+
});
718774
});
719775

720776
describe('Handles lists and nullability', () => {
@@ -882,6 +938,32 @@ describe('Execute: Handles inputs', () => {
882938
});
883939
});
884940

941+
it('limits errors when there are more than 50 errors of does not allow non-null lists of non-nulls to contain null.', () => {
942+
const doc = `
943+
query ($input: [String!]!) {
944+
nnListNN(input: $input)
945+
}
946+
`;
947+
const largeList = new Array(100).fill(null);
948+
const result = executeQuery(doc, { input: largeList });
949+
950+
const expectedErrors = [];
951+
for (let idx = 0; idx < 50; ++idx) {
952+
expectedErrors.push({
953+
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}].`,
954+
locations: [{ line: 2, column: 16 }],
955+
});
956+
}
957+
expectedErrors.push({
958+
message:
959+
'Too many errors processing variables, error limit reached. Execution aborted.',
960+
});
961+
962+
expect(result).to.deep.equal({
963+
errors: expectedErrors,
964+
});
965+
});
966+
885967
it('does not allow invalid types to be used as values', () => {
886968
const doc = `
887969
query ($input: TestType!) {
@@ -919,6 +1001,43 @@ describe('Execute: Handles inputs', () => {
9191001
],
9201002
});
9211003
});
1004+
1005+
it('limits errors when there are too many unknown types to be used as values', () => {
1006+
let queryFields = '';
1007+
let fieldArguments = '';
1008+
const valueObject = {};
1009+
const errors = [];
1010+
for (let index = 0; index < 100; ++index) {
1011+
queryFields += `$input${index}: UnknownType!`;
1012+
fieldArguments = `input${index}: $input${index}`;
1013+
if (index !== 99) {
1014+
queryFields += ',';
1015+
fieldArguments += ',';
1016+
}
1017+
valueObject[`input${index}`] = 'whoknows';
1018+
if (index < 50) {
1019+
const column = index < 10 ? 25 + index * 22 : 25 + index * 23 - 9;
1020+
errors.push({
1021+
message: `Variable "$input${index}" expected value of type "UnknownType!" which cannot be used as an input type.`,
1022+
locations: [{ line: 2, column }],
1023+
});
1024+
}
1025+
}
1026+
errors.push({
1027+
message:
1028+
'Too many errors processing variables, error limit reached. Execution aborted.',
1029+
});
1030+
const doc = `
1031+
query (${queryFields}) {
1032+
fieldWithOneHundredInputs(${fieldArguments})
1033+
}
1034+
`;
1035+
const result = executeQuery(doc, valueObject);
1036+
1037+
expect(result).to.deep.equal({
1038+
errors,
1039+
});
1040+
});
9221041
});
9231042

9241043
describe('Execute: Uses argument default values', () => {

‎src/execution/values.js

+29-2
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ export function getVariableValues(
4545
): CoercedVariableValues {
4646
const errors = [];
4747
const coercedValues = {};
48+
const maxErrors = 50;
49+
let errorLimitReached = false;
4850
for (let i = 0; i < varDefNodes.length; i++) {
4951
const varDefNode = varDefNodes[i];
5052
const varName = varDefNode.variable.name.value;
@@ -61,6 +63,10 @@ export function getVariableValues(
6163
varDefNode.type,
6264
),
6365
);
66+
if (errors.length === maxErrors) {
67+
errorLimitReached = true;
68+
break;
69+
}
6470
} else {
6571
const hasValue = hasOwnProperty(inputs, varName);
6672
const value = hasValue ? inputs[varName] : undefined;
@@ -81,6 +87,10 @@ export function getVariableValues(
8187
varDefNode,
8288
),
8389
);
90+
if (errors.length === maxErrors) {
91+
errorLimitReached = true;
92+
break;
93+
}
8494
} else if (hasValue) {
8595
if (value === null) {
8696
// If the explicit value `null` was provided, an entry in the coerced
@@ -92,19 +102,36 @@ export function getVariableValues(
92102
const coerced = coerceValue(value, varType, varDefNode);
93103
const coercionErrors = coerced.errors;
94104
if (coercionErrors) {
95-
for (const error of coercionErrors) {
105+
const reachesErrorLimit =
106+
coercionErrors.length + errors.length >= maxErrors;
107+
const publishedErrors = reachesErrorLimit
108+
? coercionErrors
109+
: coercionErrors.slice(0, maxErrors - errors.length);
110+
for (const error of publishedErrors) {
96111
error.message =
97112
`Variable "$${varName}" got invalid value ${inspect(value)}; ` +
98113
error.message;
99114
}
100-
errors.push(...coercionErrors);
115+
errors.push(...publishedErrors);
116+
if (reachesErrorLimit || coerced.errorLimitReached) {
117+
errorLimitReached = true;
118+
break;
119+
}
101120
} else {
102121
coercedValues[varName] = coerced.value;
103122
}
104123
}
105124
}
106125
}
107126
}
127+
if (errorLimitReached) {
128+
errors.push(
129+
new GraphQLError(
130+
'Too many errors processing variables, error limit reached. Execution aborted.',
131+
),
132+
);
133+
}
134+
108135
return errors.length === 0
109136
? { errors: undefined, coerced: coercedValues }
110137
: { errors, coerced: undefined };

‎src/utilities/__tests__/coerceValue-test.js

+84
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@ import {
1616

1717
function expectValue(result) {
1818
expect(result.errors).to.equal(undefined);
19+
expect(result.errorLimitReached).to.equal(undefined);
1920
return expect(result.value);
2021
}
2122

2223
function expectErrors(result) {
2324
expect(result.value).to.equal(undefined);
25+
expect(result.errorLimitReached).to.not.equal(undefined);
2426
const messages = result.errors && result.errors.map(error => error.message);
2527
return expect(messages);
2628
}
@@ -255,13 +257,63 @@ describe('coerceValue', () => {
255257
]);
256258
});
257259

260+
it('limits errors for too many invalid fields', () => {
261+
const TestInputBigObjectConfig = {
262+
name: 'TestInputBigObject',
263+
fields: {},
264+
};
265+
const valueObject = {};
266+
const errors = [];
267+
for (let index = 0; index < 100; ++index) {
268+
TestInputBigObjectConfig.fields[`field${index}`] = {
269+
type: GraphQLNonNull(GraphQLInt),
270+
};
271+
valueObject[`field${index}`] = 'abc';
272+
if (index < 50) {
273+
errors.push(
274+
`Expected type Int at value.field${index}. Int cannot represent non-integer value: "abc"`,
275+
);
276+
}
277+
}
278+
const TestInputBigObject = new GraphQLInputObjectType(
279+
TestInputBigObjectConfig,
280+
);
281+
const result = coerceValue(valueObject, TestInputBigObject);
282+
expectErrors(result).to.deep.equal(errors);
283+
expect(result.errorLimitReached).to.equal(true);
284+
});
285+
258286
it('returns error for a missing required field', () => {
259287
const result = coerceValue({ bar: 123 }, TestInputObject);
260288
expectErrors(result).to.deep.equal([
261289
'Field value.foo of required type Int! was not provided.',
262290
]);
263291
});
264292

293+
it('limits errors for too many missing required fields', () => {
294+
const TestInputBigObjectConfig = {
295+
name: 'TestInputBigObject',
296+
fields: {},
297+
};
298+
const errors = [];
299+
for (let index = 0; index < 100; ++index) {
300+
TestInputBigObjectConfig.fields[`field${index}`] = {
301+
type: GraphQLNonNull(GraphQLInt),
302+
};
303+
if (index < 50) {
304+
errors.push(
305+
`Field value.field${index} of required type Int! was not provided.`,
306+
);
307+
}
308+
}
309+
const TestInputBigObject = new GraphQLInputObjectType(
310+
TestInputBigObjectConfig,
311+
);
312+
const result = coerceValue({}, TestInputBigObject);
313+
expectErrors(result).to.deep.equal(errors);
314+
expect(result.errorLimitReached).to.equal(true);
315+
});
316+
265317
it('returns error for an unknown field', () => {
266318
const result = coerceValue(
267319
{ foo: 123, unknownField: 123 },
@@ -272,6 +324,22 @@ describe('coerceValue', () => {
272324
]);
273325
});
274326

327+
it('limits errors for too many unkown fields', () => {
328+
const valueObject = { foo: 123 };
329+
const errors = [];
330+
for (let index = 0; index < 100; ++index) {
331+
valueObject[`field${index}`] = 'string';
332+
if (index < 50) {
333+
errors.push(
334+
`Field "field${index}" is not defined by type TestInputObject.`,
335+
);
336+
}
337+
}
338+
const result = coerceValue(valueObject, TestInputObject);
339+
expectErrors(result).to.deep.equal(errors);
340+
expect(result.errorLimitReached).to.equal(true);
341+
});
342+
275343
it('returns error for a misspelled field', () => {
276344
const result = coerceValue({ foo: 123, bart: 123 }, TestInputObject);
277345
expectErrors(result).to.deep.equal([
@@ -334,5 +402,21 @@ describe('coerceValue', () => {
334402
const result = coerceValue([42, [null], null], TestNestedList);
335403
expectValue(result).to.deep.equal([[42], [null], null]);
336404
});
405+
406+
it('returns an error array limited to 50 errors and limit reached flag is true', () => {
407+
const value = [];
408+
const errors = [];
409+
for (let index = 0; index < 100; ++index) {
410+
value.push(['string']);
411+
if (index < 50) {
412+
errors.push(
413+
`Expected type Int at value[${index}][0]. Int cannot represent non-integer value: "string"`,
414+
);
415+
}
416+
}
417+
const result = coerceValue(value, TestNestedList);
418+
expectErrors(result).to.deep.equal(errors);
419+
expect(result.errorLimitReached).to.equal(true);
420+
});
337421
});
338422
});

0 commit comments

Comments
 (0)