search

syrusakbary / promise

Ultra-performant Promise implementation in Python
MIT License
362 stars 76 forks source link

Make Promise and Dataloader thread-safe #81

Closed jnak closed 4 years ago

jnak commented 4 years ago

Hi @syrusakbary,

This fixes the thread-safety issues in Promise and Dataloader that have been reported by https://github.com/syrusakbary/promise/pull/80, https://github.com/syrusakbary/promise/pull/70 and https://github.com/syrusakbary/promise/issues/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 then functions) 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 GraphQL viewer field may resolve to the wrong logged-in user and may expose very sensitive informations. You can reproduce the issue by running the test test_promise_thread_safety on master.

2. Dataloader issue

Currently Dataloader batches load calls from different threads on one single thread. While it seems it could be a good idea to globally batch load calls, this currently leads to same hard-to-reproduce concurrency bugs. For example, Dataloader will run all the load promises on the thread that happens to run dispatch_queue_batch, causing the same issue as #1. You can reproduce the issue by running the test test_dataloader_thread_safety on 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 https://github.com/syrusakbary/promise/pull/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

jnak commented 4 years ago

@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