Bump Promise version

[jnak]

Version 2.3.0 of Promise contains a fix for a critical security bug that may impact all pre-3.0 GraphQL servers running in multi-threaded mode, including all WSGI applications.

When a multi-threaded GraphQL server handles multiple concurrent requests, the execution of requests may be moved across threads. If a field resolver relies on a thread-scoped global variable such as Flask's or Django's contexts, the function will resolve to an incorrect value. For example, a GraphQL viewer field may resolve to the wrong logged-in user and may expose very sensitive informations. You can find a reproduction of this bug here.

This should resolve graphql-python/flask-graphql#56 and graphql-python/flask-graphql#43.

[jnak]

The build is failing because of a flaky test in test_executor_thread::test_evaluates_mutations_serially. The issue comes from the fact that ThreadExecutor incorrectly assumes that Promise are thread-safe.

With the fix in promise==2.3, promises are no longer being moved unexpectedly across threads while being resolved. But it does NOT make the Promise class completely thread-safe. In other words, you cannot freely access and manipulate an instance of a Promise from multiple threads, like the ThreadExecutor does when it schedules the promise on a new thread and then return the promise to the main thread where get is called (see here and here). It would be a big lift to support that and I don't see the value at all. If you want to serve different requests from multiple threads, it's much simpler to call graphql.execute on a worker thread (vs having graphql.execute spawn off and manage threads) like WSGI apps do. Honestly, I think it would be better off to delete this whole ThreadExecutor class as it is deeply flawed.

I don't think we should hold on this PR because of this because it's completely unrelated. Please let me know what I can do to get this merged. Most GraphQL servers running in Flask are currently affected by this security and correctness issue.

[Cito]

I agree we should not waste time with this, as the ThreadExecutor is not supported in v3 anyway.

We should mark the failing test with @pytest.mark.xfail and add a comment explaining why it fails.

Regarding the version number bump of graphql-core, I think we should also include #234 in the new version. These deprecation warnings are annoying and misleading. We should fix the API before releasing a new minor version. I can care about that - what do you think?

[jnak]

@Cito Sounds good.

The build is still failing because of another flaky test: test_builds_a_schema_with_field_arguments_with_default_values. This is not related to my changes since this test also fails on another PR / branch. It seems to be a known issue since you've tried to fix it recently. Should I disable this test as well?

[Cito]

@jnak Ok, found why that test is flaky. One problem is that the default container type for GraphQLInputObjectFields is dict, not OrderedDict. The other problem is that the introspection runs two times, and the default value is converted to a string. So the second time the information that the default value is an ordered dict gets lost, and the string representation might change. I will solve the problem by using a dict with a single value only. So you don't need to disable the test.

[jnak]

@Cito That was it. I had to update graphql/utils/value_from_ast.py so it uses an OrderedDict as well. Thanks!

[Cito]

@jnak Great, that should do the trick and is even better. Should be released as 2.3.0 though, because this is a minor change of behavior.

[jnak]

@Cito Is there anything blocking this to be released under 2.3.0? If not, I think it would be great from a security perspective to release this as soon as possible.

[Cito]

Ok, I'll browse through the issues later today and see if there is anything we should include in the release.

[Cito]

Version 2.3.0 is out now.