Fix web UI when using session-based CSRF

[seawolf42]

Note: Before submitting this pull request, please review our contributing guidelines.

Description

Closes #6206

Pass the actual CSRF token value instead of the name of the cookie in which to find the value, and use this actual value in the X-CSRFToken header.

[seawolf42]

I believe there will need to be similar changes to the coreapi source, but I am not sure how to adapt it to behave similarly.

[tomchristie]

On first sight I'd assume this would resolve the issue when using CSRF_USE_SESSIONS=True but break it for the default case?

[seawolf42]

It shouldn't break it for the default case, as the same token value that was being sent by default is still being sent.

In the case of CSRF_USE_SESSIONS=True: The csrf cookie is not present, so instead of depending on it to get the value to inject into the response, the template includes the actual value to use, and the JS code reads it directly.

In the case of CSRF_USE_SESSIONS=False (the default): The csrf cookie is present, but instead of depending on it to get the value to inject into the response, the template includes the actual value to use, and the JS code reads it directly.

In both cases, the response blob still contains the CSRF token the same way it did before; it's just being read from a raw value instead of being read from a cookie which may or may not be present.

[seawolf42]

@tomchristie are you referring to the {% if request %} condition in the base.html file? I'm not familiar enough with the architecture to know if/when that would be false, so I could use an assist there.

[codecov-io]

Codecov Report

Merging #6207 into master will decrease coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6207      +/-   ##
==========================================
- Coverage   96.18%   95.85%   -0.33%     
==========================================
  Files         128      129       +1     
  Lines       17645    17862     +217     
  Branches     1460     1476      +16     
==========================================
+ Hits        16972    17122     +150     
- Misses        465      538      +73     
+ Partials      208      202       -6

Impacted Files	Coverage Δ
tests/test_templates.py	100% <100%> (ø)	⬆️
rest_framework/renderers.py	76.86% <0%> (-7.99%)	⬇️
tests/test_schemas.py	94.03% <0%> (-0.16%)	⬇️
tests/test_templatetags.py	100% <0%> (ø)	⬆️
tests/test_viewsets.py	100% <0%> (ø)	⬆️
tests/test_utils.py	100% <0%> (ø)	⬆️
rest_framework/decorators.py	98.29% <0%> (ø)	⬆️
...st_framework/management/commands/generateschema.py	0% <0%> (ø)
rest_framework/fields.py	95.18% <0%> (ø)	⬆️
tests/test_fields.py	98.41% <0%> (+0.01%)	⬆️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d976ac5...c71e472. Read the comment docs.

[seawolf42]

@carltongibson I updated a unit test as we discussed yesterday to show that at least the base template includes the csrfToken data. I would also like to add a test for the positive case, something like:

def test_base_template_with_context(mocker):
    csrf_token_mock = mocker.patch('django.template.defaulttags.csrf_token', return_value='<TOKEN>')
    user = User.objects.create(username='abc')
    Token.objects.create(user=user)
    factory = RequestFactory()
    request = factory.get('/')
    request.user = user
    result = render(request, 'rest_framework/base.html')
    csrf_token_mock.assert_called_once()
    assert re.search(r'\bcsrfToken: "<TOKEN>"', result.content)

... but the project doesn't include pytest-mock nor does it include unittest.mock. Do you have any reason why I couldn't add either of these to the project, or is there a better way to test this?

[carltongibson]

Hi @seawolf42.

Similar comes up on Django re CSRF_COOKIE_HTTPONLY
https://code.djangoproject.com/ticket/29879. (You can't read the cookie value there either.)

unittest.mock is part of the standard library. You can use it. 👍

[seawolf42]

@carltongibson but mock is not part of the standard library for python < 3... 😄

I'll take a look at 29879.

[seawolf42]

@carltongibson disregard, I figured out a test I could do without needing to mock anything.

[carltongibson]

but mock is not part of the standard library for python < 3

I'd be happy for you to skip the test on PY2. (We're dropping it for 3.10 anyhow... c.f. #6230)

[seawolf42]

@carltongibson I ended up writing a test that didn't require any mocks in the first place, so that has gone away. The only open question now is whether I need to also make changes to the core API itself as part of this PR or whether the note in the docs is enough.

[carltongibson]

OK. Thanks! I will have a look tomorrow. Good effort!

[carltongibson]

OK. A few comments, but I think the general approach is reasonable.

[carltongibson]

Do you mean {% csrf_token %}? c.f. #3703 &co.

[seawolf42]

How very odd... I am sure I used it this way when I was first figuring things out, clearly I was mistaken. OK, I'll have to re-work one test too, will take care of that in the next day or two.

[seawolf42]

Oh, no, I do want {{ csrf_token }}. I don't want the entire HTML tag, just the value of the token.

[carltongibson]

Good, and TemplateHTMLRenderer will create a RequestContext, so csrf_token will be in the context. Fine. 👍

[carltongibson]

Again {% csrf_token %}? And do we need the if request check?

[seawolf42]

We do need that check, because there is currently a unit test for rendering with no request and in that case the csrf_token call throws an exception (see https://code.djangoproject.com/ticket/29785, which you closed as "by design").

[seawolf42]

OK, read the comments out of order. Here we don't need the {% if request %} check because (presumably) there will always be a request when you load an admin page. In the base.html case, however, the check is only there because of the exception thrown if there is no request, this is to satisfy test_templates.test_base_template_with_no_context().

[carltongibson]

OK. I think this looks good to me.

(Just await some other 👀)

[seawolf42]

Excellent, thanks for the help!

[carltongibson]

@seawolf42 Thanks for your effort!

[rpkilby]

I'm not incredibly familiar with how Django's CSRF mechanism works since I've never really dug into it.

Given my ignorance on the subject, my concern would be that this PR fixes the problem, but possibly introduces some CSRF-related vulnerability. I assume there's a reason the token is passed in a cookie instead of it's value being embedded in the template?

[seawolf42]

@rpkilby that's a reasonable concern. I'll explain as best I can that this is actually OK.

Background: http://kylebebak.github.io/post/csrf-protection - read the section on "Double-Submit Cookie", the mechanism Django uses.

The key takeaway is that there are two items that matter: the CSRF token as the server knows it and the CSRF token that the request contains. They are compared, and if they're the same, the request passes. The security of this hinges on the assumption that at least one of those values cannot be randomly changed by the user. Traditionally this is handled by having a cookie value that is set by the server that comes in on every request, and a token that the client sees in a Django form and sends back as a form field. There are alternate ways to retrieve the second value, but the cookie (if using cookies) must be trusted to have only been set either by the server or by JS sent from the server so that it can be trusted as an authoritative value.

DRF has been just looking at the cookie and sending that value as the second value rather than getting it to the client as part of page loads. If the cookie is trustworthy, and the JS can see the cookie, then using it doesn't break security (since the same-origin policy means if the JS can see it the JS came from the server, which can be trusted).

Enter session-based CSRF. Note that this was introduced specifically to increase security by solving situations where cookies assigned by a subdomain in a hosting environment may be readable by other applications in that same environment (so mysite.appspot.com might be hackable by yoursite.appspot.com if you can read the cookie I've set).

In this mechanism, the authoritative value is never sent to the client in any form; it's stored in the session instead. Now reading the value from the cookie doesn't work any longer because the cookie doesn't exist. The way to get the token to the server instead now hinges on it being in the page the server gave me, and I then read the value and send it back. The only cookie that requires protection is the session cookie, reducing the attack footprint because session cookies get extra browser protections.

Note again how traditional Django handles this: instead of reading the cookie value, the client receives the token on every form as a hidden field and the server receives that back as part of the form submission, then compares that value to the cookie that was set earlier. Switching to session-based CSRF doesn't break this because the authoritative value is in the session rather than the cookie but all that matters is the hidden field value is compared to that authoritative value to pass/fail the request.

In the DRF case, this PR changes the web UI to receive the CSRF token that same way... as part of the incoming page. No other aspect of the process is impacted, so the assumption here is that the Django forms way of doing things is the best practice, and we're just emulating it.

[seawolf42]

As a side note, the switch to session-base CSRF essentially switches from "Double-Submit Cookie" to "Synchronizer Token" in the link above. This PR enables that switch with the web UI.

[rpkilby]

Hi @seawolf42 - thanks for the comprehensive writeup. I haven't had time to read through the linked articles in depth, but looking over your response, the implementation in the PR sounds 👍 to me.

DRF has been just looking at the cookie and sending that value as the second value rather than getting it to the client as part of page loads. If the cookie is trustworthy, and the JS can see the cookie, then using it doesn't break security (since the same-origin policy means if the JS can see it the JS came from the server, which can be trusted).

So my understanding here is that it doesn't matter whether the token is read from the response body (e.g., embedded in a form or in an inlined script) or read from the cookie, since the same origin policy prevents an external site from reading the contents. The token wouldn't unintentionally be exposed.

And it looks like the PR is compatible with both the cookie and session-based CSRF mechanisms.

Would it make sense to add some simple sanity tests for both settings? something like..

@override_settings(CSRF_USE_SESSIONS=False) 
def test_cookie_csrf():
    response = self.client.get(url)

    # CSRF token should be provided in page and as a cookie
    assert 'csrftoken' in response.client.cookies
    assert re.search(r'\bcsrfToken: ""', response.body.decode('utf-8'))
   

@override_settings(CSRF_USE_SESSIONS=True) 
def test_session_csrf():
    response = self.client.get(url)

    # CSRF cookie should not be set
    assert 'csrftoken' not in response.client.cookies

    # CSRF token should be provided in page and through session cookie
    assert 'session' in response.client.cookies
    assert re.search(r'\bcsrfToken: ""', response.body.decode('utf-8'))

These aren't full e2e tests, but should at least ensure that the necessary parts are provided in the response (right?).

[rpkilby]

I would still appreciate a review from @tomchristie or someone else with more experience in this area, but the PR looks good to me.

[carltongibson]

Hi @seawolf42. Right, 3.9.2 is it. 🙂

Thanks for the effort here, and welcome aboard! 🏆

[seawolf42]

Awesome, thanks!