Reject requests earlier if the non-cookie CSRF token is missing or has the wrong format

[Chris Jerdonek]

I noticed that CsrfViewMiddleware.process_view() does what seems like unnecessary work when the non-cookie CSRF token is either missing or has the wrong format (i.e. has the wrong length or contains characters that aren't allowed). Specifically, in ​these lines:

if request_csrf_token == '':
    # Fall back to X-CSRFToken, to make things easier for AJAX, and
    # possible for PUT/DELETE.
    request_csrf_token = request.META.get(settings.CSRF_HEADER_NAME, '')

request_csrf_token = _sanitize_token(request_csrf_token)
if not _compare_masked_tokens(request_csrf_token, csrf_token):
    return self._reject(request, REASON_BAD_TOKEN)

if the request_csrf_token is missing or has the wrong format, the code will proceed inside _sanitize_token () to use Python's secrets module twice to generate both a new token and a mask for the token, but only for the purposes of calling _compare_masked_tokens() in a way that will be guaranteed to fail (since the token being passed will be brand new). And then it will call _compare_masked_tokens() with that value.

However, if the non-cookie token is missing or has the wrong format, it seems like the request can be rejected at that point outright without needing to do the work above. It doesn't seem like rejecting the request outright will reveal any sensitive information since the correct token length and allowed characters aren't secret information. (Django's security model assumes that information is publicly known.)

Another advantage of rejecting earlier is that the failure message can be more specific. Namely, instead of just using REASON_BAD_TOKEN ("CSRF token missing or incorrect"), more specific messages can be used like "CSRF token missing," "CSRF token has wrong length," and "CSRF token contains invalid characters." That could be useful in troubleshooting CSRF issues, which can sometimes be hard to troubleshoot.

A third advantage is that this will make the code easier to understand. This is because currently, it's hard to tell whether calling _sanitize_token() and _compare_masked_tokens() are actually needed for security reasons even when the CSRF token is missing or has the wrong format. (There currently aren't any comments explaining why it's needed if in fact it is.)

[Chris Jerdonek]

One way to implement this would be to change ​_sanitize_token() to raise a new internal InvalidTokenFormat exception with an appropriate reason string if the token has the wrong length or contains invalid characters, instead of calling _get_new_csrf_token(). Then, the two places that call _sanitize_token() can handle the exception differently: (1) In process_view(), the request could be rejected using the exception's message. This is similar to how process_view() now handles RejectRequest exceptions raised by _check_referer(). (2) In _get_token(), the exception could be handled by calling _get_new_csrf_token().

[Florian Apolloner]

The proposed solution sounds good to me!

[Chris Jerdonek]

PR: ​https://github.com/django/django/pull/14464

[Mariusz Felisiak <felisiak.mariusz@…>]

In ffdee8d2:

Refs #32795 -- Added CsrfViewMiddleware tests for rejecting invalid or missing tokens.

This also improves test names for test_process_request_no_csrf_cookie
and test_process_request_csrf_cookie_no_token. The logic being tested
is actually in process_view() rather than process_request(), and it's
not necessary to include the method name.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 55775891:

Fixed #32795 -- Changed CsrfViewMiddleware to fail earlier on badly formatted tokens.