construction fails for depth around > 200 / resource exhaustion?

[devkral]

Reporting issues with GraphQL-core 3

In my tests the construction of a deep request tree fails with recursion problems.
The problem is a recursive approach in the generation of the graphql request tree (this is why I created the test).

Next to denial of service it is most probably possible to cause resource exhaustion attacks by passing big graphs.

There should be two changes:

• a "stack free" (not really stack free but the recursion depth is drastically reduced) approach in generating the input graph. I did something with generators in my project: graphene-protector:
  https://github.com/devkral/graphene-protector
• a node limit after which the generation of the input graph is stopped with an error

I am not sure if the cost spec ( https://ibm.github.io/graphql-specs/cost-spec.html ) can fix this. The changes must take place while generating the requested input graph

[devkral]

Working example code for simply transform recursive function in an "stack free" call tree:

from functools import partial


def _foo(level=0, *, get_result):
    if level > 100000:
        yield level
        return
    print("1", level)
    yield partial(_foo, level=level + 1)
    print("2", get_result())
    yield level


def foo():
    result_stack = []

    fn_stack = [_foo(get_result=result_stack.pop)]

    while fn_stack:
        cur_el = fn_stack[-1]
        try:
            next_el = next(cur_el)
            if isinstance(next_el, partial):
                fn_stack.append(next_el(get_result=result_stack.pop))
            else:
                result_stack.append(next_el)
        except StopIteration:
            fn_stack.pop()
foo()

[erikwrede]

Hey there, it's best to report any particular issues with graphql-core in the corresponding repository. Please note that graphql core closely follows the reference implementation written in typescript in order to be as up to date as possible given our available maintainer time.

That's why I deem it unlikely to see any changes to the execution logic on just the python level. If you're interested in bringing this into graphql-js, it might be worthwhile to open up an RFC there and follow the process.

The limits functionality of graphene-protector look amazing. Are you interested in contributing them to strawberry-graphql or graphene directly so they're even easier to use? Feel free to contact me about both, I'm a core maintainer on both teams 😊

[Cito]

What Erik wrote. Completely changing the execution or parsing logic is out of the scope of this project.

If it really would be a significant improvement, consider discussing this in the upstream GraphQL-js project.

Also, please note that a max_tokens parameter has been added to GraphQL-js and already ported to GraphQL-core. This could also help to avoid denial of service via memory exhaustion. Have you seen that?

[devkral]

thanks for the hint. Then only the first issue is valid.
It isn't really much effort. Generators are a good way to save the current progress

[devkral]

Hey there, it's best to report any particular issues with graphql-core in the corresponding repository. Please note that graphql core closely follows the reference implementation written in typescript in order to be as up to date as possible given our available maintainer time.

That's why I deem it unlikely to see any changes to the execution logic on just the python level. If you're interested in bringing this into graphql-js, it might be worthwhile to open up an RFC there and follow the process.

The limits functionality of graphene-protector look amazing. Are you interested in contributing them to strawberry-graphql or graphene directly so they're even easier to use? Feel free to contact me about both, I'm a core maintainer on both teams 😊

if you like to, we can move it. I proposed it a few years ago if I remember right. I would really like to develop this in a team.

[devkral]

What Erik wrote. Completely changing the execution or parsing logic is out of the scope of this project.

If it really would be a significant improvement, consider discussing this in the upstream GraphQL-js project.

Also, please note that a max_tokens parameter has been added to GraphQL-js and already ported to GraphQL-core. This could also help to avoid denial of service via memory exhaustion. Have you seen that?

I had some time to look through the code of the upstream project:

they seem to not to use a recursive pattern for parsing inputs, so they should be safe.
The max_token parameter is nice but doesn't prevent depths around 200 except it is configured very restrictive

They seem to be affected the same. I misread the code.

[Cito]

Maybe, instead of or in addition to max_tokens, there should be something like max_level? Is that something we should suggest?

[devkral]

Maybe, instead of or in addition to max_tokens, there should be something like max_level? Is that something we should suggest?

This solves a symptom and not the problem: Level > 200 depth is simply not supported.
An easy fix would just handle the crash and return an appropriate error (already done implicitly but without a nice error)

[devkral]

Note: currently there is no use in having 200 level depths of recursion but this can change if someone abuses strawberry-django filters for building really big filters.