Skip to content

Security issue - context should be copied for each request #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dpnova opened this issue Sep 3, 2017 · 4 comments
Closed

Security issue - context should be copied for each request #8

dpnova opened this issue Sep 3, 2017 · 4 comments

Comments

@dpnova
Copy link
Contributor

dpnova commented Sep 3, 2017

https://github.com/graphql-python/sanic-graphql/blob/master/sanic_graphql/graphqlview.py#L51

If you set context in the View ctor a class level attribute gets set. This is fine, except the first request is put into the object and never overwritten later. This ends up serving the first request object in the context of all subsequent requests.

I'm going to patch now. Just thought I'd drop this here to see if anyone has a good reason for request to not be overwritten in context every request. Or more appropriately, for the provided context to be copied each request.

@dpnova
Copy link
Contributor Author

dpnova commented Sep 3, 2017

graphql-python/flask-graphql@d728f80

I suppose I should be using app.db to share my db connection... not a big fan of that style though.

@dpnova
Copy link
Contributor Author

dpnova commented Sep 3, 2017

I'm pretty sure flask uses a bunch of thread locals for things like this, which highlights the importance of the approach taken here.

@dpnova
Copy link
Contributor Author

dpnova commented Sep 3, 2017

Another suggested approach could be for context to be a callable that returns the default context, thus less mutable state hanging around.

grazor added a commit that referenced this issue Sep 4, 2017
Copy the initial context rather than updating it. #8
@grazor
Copy link
Member

grazor commented Sep 4, 2017

@dpnova, thanks for pull request, it clearly was an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants