Add optional maxDepth and maxAliases execution options#4666
Add optional maxDepth and maxAliases execution options#4666eddieran wants to merge 2 commits intographql:16.x.xfrom
Conversation
Add opt-in depth and alias limits to the execution engine to mitigate denial-of-service attacks via deeply nested queries and alias bombing. - maxDepth: limits the field nesting depth during execution. When a field exceeds the configured depth, a GraphQLError is raised and the parent field resolves to null (standard error handling). - maxAliases: limits the number of response keys (including aliases) per selection set. When exceeded, a GraphQLError is raised before the selection set is executed. Both options are undefined by default, preserving full backwards compatibility. They are passed via ExecutionArgs.options alongside the existing maxCoercionErrors option. Fixes graphql#4662
|
|
@eddieran is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel. A member of the Team first needs to authorize it. |
yaacovCR
left a comment
There was a problem hiding this comment.
Maybe we could consider adding a fieldDepth property to FieldDetails of type number? Then we wouldn't have to keep recalculating the path length?
Addresses review feedback: rather than walking the Path linked list at every executeField invocation, thread the current field depth as an explicit parameter through executeFields/executeField/completeValue and its helpers. completeObjectValue increments depth when descending into sub-selections; list items and abstract-type resolution keep the same depth as their parent field. This changes the depth check from O(depth) per field resolution to O(1), while preserving identical semantics (list indices still do not count toward depth).
|
Thanks for the review, @yaacovCR! Since This turns the depth check into O(1) per field resolution instead of O(depth) walk of the Path linked list, while preserving identical semantics (list indices still don't count). All 12 new tests and the full 1972-test suite still pass. Pushed in f9d67dc. Happy to take further suggestions, e.g. if you'd prefer a different naming or placement. |
yaacovCR
left a comment
There was a problem hiding this comment.
This looks great to me, comments/suggestions inline.
Looping in @benjie with regards to naming as he is driving the Golden Path initiative (graphql/graphql-wg#1887 https://github.com/graphql/golden-path-wg).
Gentle nudge also about the CLA!
(CI also notes there is a prettier issue).
From benchmarking on my machine ran from the 17.x.x branch:
npm run benchmark -- benchmark/introspectionFromSchema-benchmark.js benchmark/list-sync-benchmark.js benchmark/list-async-benchmark.js benchmark/object-async-benchmark.js --revs pr/4666 v16.13.2
| path: Path, | ||
| depth: number, | ||
| ): PromiseOrValue<unknown> { | ||
| // Check depth limit before resolving the field. |
There was a problem hiding this comment.
If the depth limit check was in completeObjectValue (and at the root) rather than at the field, right after field collection, we could return all the collected fields/subfields. We could also with this change allow for setting a limit of 0 which should fail everything, not be so useful, but would protects us in that edge case from an error message in that edge case of uncertain use that would read "Query depth limit of 0 exceeded, found depth: 2." when it might be expected to read: "Query depth limit of 0 exceeded, found depth: 1."
| * When exceeded, a GraphQLError is thrown for the offending field. | ||
| * No limit is applied when undefined (the default). | ||
| */ | ||
| maxDepth?: number; |
There was a problem hiding this comment.
We could consider calling this option maxResultDepth as we may later wish to introduce a separate maximum inline/named fragment depth with respect to field collection.
| * When exceeded, a GraphQLError is thrown before executing the selection set. | ||
| * No limit is applied when undefined (the default). | ||
| */ | ||
| maxAliases?: number; |
There was a problem hiding this comment.
| maxAliases?: number; | |
| maxResponseNames?: number; |
The idea is that we want a maximum breadth at an object level, whether or not aliases are used.
I think "response name" would be the official erm => https://spec.graphql.org/draft/#sec-Field-Alias
(This would need to be renamed throughout the code/comments.)
| if (exeContext.maxAliases !== undefined) { | ||
| const aliasCount = fields.size; | ||
| if (aliasCount > exeContext.maxAliases) { | ||
| // Collect nodes for the error location from the first field node of |
There was a problem hiding this comment.
Not sure, but I think we would want to include all nodes, not just those above the maximum, it's not the fault of a later node if a new node is added to a operation prior to it and causes it to fail? And we would not be able to tell which is the real offending node. Alternatively, we could just list the parent with too many sub-response names. Not sure how much value we actually add by including all of the nodes causing the response names limit to be breached, it may be more helpful to highlight the offending parent (considering the list of nodes would be expected to be quite long).
| // each response key beyond the limit. | ||
| const nodes: Array<FieldNode> = []; | ||
| for (const [, fieldNodes] of fields) { | ||
| nodes.push(fieldNodes[0]); |
There was a problem hiding this comment.
If we include all the offending response names, for consistency, I would assume we would need to include all of the fieldNodes, not just the first. This array is the set of overlapping fieldNodes that have been coalesced into one response name, but if we think we're helping by pointing out all the nodes within the operation that cause this limit to be breached, I think we would just confuse things by not listing all the nodes that are part of the problem. If we include just the first, the removal of that node would not cause the error to be avoided if there were other nodes with that response name.
(As above, not sure if we actually need to include all of the offending nodes rather than just the parent.)
Summary
Fixes #4662 — the execution engine has no built-in depth or complexity limits, allowing deep recursive queries and alias-bombing to cause denial-of-service via resolver amplification.
This PR adds two opt-in execution options:
maxDepth— limits the field nesting depth during execution. When a field at depth >maxDepthis encountered, aGraphQLErroris raised and the parent field resolves tonull(standard nullable error handling). List indices do not count toward depth.maxAliases— limits the number of response keys (including aliases) per selection set. This prevents alias-bombing attacks where thousands of aliases for the same field bypass depth-based defenses. When exceeded, aGraphQLErroris raised before the selection set executes.Both options are
undefinedby default — no limits are applied and existing behavior is fully preserved. They are passed viaExecutionArgs.optionsalongside the existingmaxCoercionErrors:Implementation details
executeFieldby walking thePathlinked list (only counting string keys, not list indices)executeOperation(root fields) andcompleteObjectValue(sub-selections) after field collectionTest plan