Make Promise and Dataloader thread-safe #81
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
syrusakbary
approved these changes
Dec 14, 2019
jnak
force-pushed
the
concurrency-bug
branch
from
b07260e
to
c519261
Compare
|
@syrusakbary Thanks for merging this. Would you mind releasing a new version on PyPi today? It's critical that we roll out this security patch as soon as possible. Thanks |
This was referenced Dec 18, 2019
alex-verve
pushed a commit
to alex-verve/graphene
that referenced
this pull request
May 15, 2020
it has following fixes: * Make Promise and Dataloader thread-safe syrusakbary/promise#81 * Fix memory leak on dynamically created types syrusakbary/promise#76 * and more https://github.com/syrusakbary/promise/releases
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @syrusakbary,
This fixes the thread-safety issues in Promise and Dataloader that have been reported by #80, #70 and #68. These are major security issues that are potentially impacting all Graphql servers that run in threaded mode, including all wsgi applications (see reproduction).
1. Promise issue
Currently promise tasks (ie
thenfunctions) can be executed on a different thread from the one they were scheduled on. If the tasks rely on any thread-scoped global variables such as Flask's or Django's contexts, the tasks will be executed with incorrect values. For example, a GraphQLviewerfield may resolve to the wrong logged-in user and may expose very sensitive informations. You can reproduce the issue by running the testtest_promise_thread_safetyon master.2. Dataloader issue
Currently Dataloader batches
loadcalls from different threads on one single thread. While it seems it could be a good idea to globally batchloadcalls, this currently leads to same hard-to-reproduce concurrency bugs. For example, Dataloader will run all theloadpromises on the thread that happens to rundispatch_queue_batch, causing the same issue as #1. You can reproduce the issue by running the testtest_dataloader_thread_safetyon master.Next steps
It is critical that we release this patch as soon as possible since all GraphQL servers running in threaded-mode are currently impacted (see reproduction).
Even though all the tests pass on this branch, the build is currently failing due to some mypy errors. If you wish to have build errors resolved before merging this, I recommend you first merge #79 since it is very safe to merge and it fixes all those lint issues. I rebased my changes on top of that branch to make sure the build will pass.
Please let me know what I can do to help get this released as fast as possible.
Thanks,
J