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

Only set missing csrftoken cookie if needed by current page #7

Closed
simonw opened this issue Jun 5, 2020 · 1 comment
Closed

Only set missing csrftoken cookie if needed by current page #7

simonw opened this issue Jun 5, 2020 · 1 comment
Labels
enhancement New feature or request

Comments

@simonw
Copy link
Owner

simonw commented Jun 5, 2020

See comment on simonw/datasette#798 (comment) - right now this middleware sets the csrftoken cookie if it is missing on EVERY page.

This is bad, because it doesn't take caching into account. Pages should not be cached by Varnish/CloudFlare etc if they are setting a secret cookie value!

Instead, we should do what Django does. Here's a snippet from the 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

@simonw simonw added the enhancement New feature or request label Jun 5, 2020
@simonw
Copy link
Owner Author

simonw commented Jun 5, 2020

Since the token is passed as part of the scope, I'm going to change scope["csrftoken"] from a string value into a function which returns a string value.

That way the outer middleware can detect if the csrftoken was accessed or displayed on a page, which means it knows if it should set the cookie on the response or not.

@simonw simonw closed this as completed in 849b397 Jun 5, 2020
simonw added a commit that referenced this issue Jun 5, 2020
simonw added a commit to simonw/datasette that referenced this issue Jun 5, 2020
- Use new csrftoken() function, refs simonw/asgi-csrf#7
- Check for Vary: Cookie hedaer, refs simonw/asgi-csrf#8

Refs #793 and #798
simonw added a commit to simonw/datasette that referenced this issue Jun 5, 2020
Closes #793.

* Rename RequestParameters to MultiParams, refs #799
* Allow tuples as well as lists in MultiParams, refs #799
* Use csrftokens when running tests, refs #799
* Use new csrftoken() function, refs simonw/asgi-csrf#7
* Check for Vary: Cookie hedaer, refs simonw/asgi-csrf#8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant