Changes between Initial Version and Version 1 of Ticket #32796


Ignore:
Timestamp:
May 29, 2021, 6:37:41 PM (3 years ago)
Author:
Chris Jerdonek
Comment:

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #32796 – Description

    initial v1  
    33I noticed in `CsrfViewMiddleware.process_view()` that if the CSRF cookie has the wrong format (i.e. wrong length or contains invalid characters), then the code will do a fair amount of unnecessary work. Specifically, the code will proceed inside `_get_token()` at [https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L362 this line] to use Python's `secrets` module twice to generate both a new token and a mask for the token. But this new token will only be used for the purposes of later calling `_compare_masked_tokens()` in a way that will be guaranteed to fail (since the cookie being used will be brand new and so won't match). And then it will call `_compare_masked_tokens()` with that value.
    44
    5 Instead, if the CSRF cookie is found at that line to have the wrong format, the middleware could reject the request outright similar to how #32795 does it if the token has the wrong format. I think this will simplify `CsrfViewMiddleware` and make it easier to understand because it will eliminate a number of steps that aren't needed for security. In particular, one thing this will do is cut down the number of places where `_get_new_csrf_token()` is called, which will make it clearer where a new value is really needed / used. Similar to #32795, it will also make troubleshooting easier because the rejection messages will be more specific.
     5Instead, if the CSRF cookie is found at that line to have the wrong format, the middleware could reject the request outright similar to how #32795 does it if the token has the wrong format (as well as similar to how the code currently handles a ''missing'' cookie in the [https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L363-L367 lines after]). I think this will simplify `CsrfViewMiddleware` and make it easier to understand because it will eliminate a number of steps that aren't needed for security. In particular, one thing this will do is cut down on the number of places where `_get_new_csrf_token()` is called, which will make it clearer where a new value is really needed / used. Similar to #32795, it will also make troubleshooting easier because the rejection messages will be more specific.
    66
    7 I think this could be implemented as follows. After #32795 is merged, [https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L193 _get_token()] could be changed to allow `InvalidTokenFormat` to bubble up instead of handling it. Then the `InvalidTokenFormat` exception could be handled differently in the two places `_get_token()()` is called: (1) In `process_request()`, it could be handled by calling `_get_new_csrf_token()` (`_get_token()`'s current behavior). (2) In `process_view()`, it could be handled similar to how #32795 handles it. Namely, reject the request using the `InvalidTokenFormat`'s reason string.
     7I think this could be implemented as follows. After #32795 is merged, [https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L193 _get_token()] could be changed to allow `InvalidTokenFormat` to bubble up instead of handling it. Then the `InvalidTokenFormat` exception could be handled differently in the two places `_get_token()` is called: (1) In `process_request()`, it could be handled by calling `_get_new_csrf_token()` (`_get_token()`'s current behavior). (2) In `process_view()`, it could be handled similar to how #32795 handles it. Namely, reject the request using the `InvalidTokenFormat`'s reason string.
Back to Top