Skip to content
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

CSRF protection #798

Merged
merged 6 commits into from
Jun 5, 2020
Merged

CSRF protection #798

merged 6 commits into from
Jun 5, 2020

Conversation

simonw
Copy link
Owner

@simonw simonw commented Jun 5, 2020

Refs #793

@simonw
Copy link
Owner Author

simonw commented Jun 5, 2020

Needs unit tests.

More importantly: needs very, very careful consideration of how this plays with HTTP caching.

@simonw
Copy link
Owner Author

simonw commented Jun 5, 2020

I don't want to set a cookie on a page response that is being cached.

Right now the ASGI middleware will be doing exactly that, which is bad.

But how do I get certainty that when you load a page with a form that will be CSRF protected you have been served the cookie?

Maybe those pages should do something explicit to the request object indicating that the cookie is needed?

That works for Datasette (since it has mutable request objects) but I'm not sure how it would work in the asgi-csrf pure ASGI middleware context.

@simonw
Copy link
Owner Author

simonw commented Jun 5, 2020

Django docs on CSRF and caching: https://docs.djangoproject.com/en/3.0/ref/csrf/#caching

If the csrf_token template tag is used by a template (or the get_token function is called some other way), CsrfViewMiddleware will add a cookie and a Vary: Cookie header to the response. This means that the middleware will play well with the cache middleware if it is used as instructed

So the cookie is only set for pages that included a hidden csrftoken form field! This could work.

@simonw
Copy link
Owner Author

simonw commented Jun 5, 2020

I'm solving the compatibility with caching problem in this ticket: simonw/asgi-csrf#7

@simonw
Copy link
Owner Author

simonw commented Jun 5, 2020

Add unit tests illustrating the Vary: Cookie header and I'm done here.

- Use new csrftoken() function, refs simonw/asgi-csrf#7
- Check for Vary: Cookie hedaer, refs simonw/asgi-csrf#8

Refs #793 and #798
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

Successfully merging this pull request may close these issues.

1 participant