-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
No CSRF cookie for docs pages #4153
No CSRF cookie for docs pages #4153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most doc pages are served directly from nginx, so is this a common issue in production?
I'd also imagine the footer API could be setting cookies, and would likely be called on all pages.
readthedocs/core/views/serve.py
Outdated
@@ -144,6 +147,7 @@ def _serve_file(request, filename, basepath): | |||
return response | |||
|
|||
|
|||
@csrf_exempt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include a comment here about why we're making these exempt (no cookies on doc pages)
Looks like the doc pages & footer don't return Cookies at all. So I guess this is just catching the last of the cookie-setting views that we have? |
Looks like the 404 page also sets a cookie:
|
It doesn't appear to be.
This is a good catch! This is part of the problem. |
I tracked this problem down a little deeper. It looks like the cookie getting set unnecessarily is 100% due to the 404 page setting the CSRF cookie. The 404 page sets the CSRF cookie because of the set language form in the footer of the 404 page. I failed to notice this partially because some of our docs (notably https://docs.readthedocs.io/en/latest/) always have a 404 due to this (unnecessary?) line but others don't. I see a couple solutions:
Thoughts? |
Removing the form from the 404 page seems like a simple solution, if there's an easy way to do it. |
- Without the language select form, no CSRF cookies will be sent
I removed the language select form from the error pages. This removes setting the cookie. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an elegant solution. 👍
Just want to mention that we might need the same for the corporate site on the same files:
The other templates modified in PRs are not override in corporate, so nothing to do I suppose. |
I'm not as concerned if corporate hosted docs set a CSRF cookie although it may still be good to look at. Typically, we have a relationship with people visiting corporate docs. |
This marks docs pages served by Django as CSRF exempt. This should prevent a CSRF cookie from being set (it does in dev at least).