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

No CSRF cookie for docs pages #4153

Merged
merged 4 commits into from May 30, 2018

Conversation

davidfischer
Copy link
Contributor

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).

@davidfischer davidfischer requested a review from a team May 26, 2018 19:48
Copy link
Member

@ericholscher ericholscher left a 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.

@@ -144,6 +147,7 @@ def _serve_file(request, filename, basepath):
return response


@csrf_exempt
Copy link
Member

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)

@ericholscher
Copy link
Member

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?

@ericholscher
Copy link
Member

Looks like the 404 page also sets a cookie:

-> curl -IL http://docs.readthedocs.io/en/latest/404/
HTTP/1.1 404 Not Found
Server: nginx/1.10.3 (Ubuntu)
Vary: Accept-Encoding
Vary: Cookie, Accept-Language
Content-Type: text/html; charset=utf-8
Date: Tue, 29 May 2018 15:20:25 GMT
Transfer-Encoding: chunked
Content-Language: en
Connection: Keep-Alive
Set-Cookie: csrftoken=3WLWur0BF8UXb468SG95cVi8ShE4tmGY; expires=Tue, 28-May-2019 15:20:25 GMT; httponly; Max-Age=31449600; Path=/

@davidfischer
Copy link
Contributor Author

the footer API could be setting cookies

It doesn't appear to be.

Looks like the 404 page also sets a cookie

This is a good catch! This is part of the problem.

@davidfischer
Copy link
Contributor Author

csrf_exempt does not do what I thought it did. It does not prevent the CSRF cookie from being set.

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:

  • Simply remove the set language form from the 404 page
  • Have a more simplified 404 page

Thoughts?

@ericholscher
Copy link
Member

Removing the form from the 404 page seems like a simple solution, if there's an easy way to do it.

@davidfischer
Copy link
Contributor Author

I removed the language select form from the error pages. This removes setting the cookie.

Copy link
Member

@ericholscher ericholscher left a 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. 👍

@humitos
Copy link
Member

humitos commented May 30, 2018

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.

@davidfischer
Copy link
Contributor Author

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.

@davidfischer davidfischer merged commit 19ff40d into readthedocs:master May 30, 2018
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.

None yet

3 participants