Opened 4 years ago
Closed 4 years ago
#32796 closed Cleanup/optimization (fixed)
Reject requests earlier if the CSRF cookie token has the wrong format
Reported by: | Chris Jerdonek | Owned by: | Chris Jerdonek |
---|---|---|---|
Component: | CSRF | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Shai Berger, Florian Apolloner | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
(This issue is similar to #32795 but for the cookie token rather than for the non-cookie token.)
I 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 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.
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 (as well as similar to how the code currently handles a missing cookie in the 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.
I think this could be implemented as follows. After #32795 is merged, _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.
Change History (8)
comment:1 by , 4 years ago
Description: | modified (diff) |
---|
comment:2 by , 4 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 4 years ago
Owner: | changed from | to
---|
comment:4 by , 4 years ago
comment:5 by , 4 years ago
Has patch: | set |
---|
comment:6 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR: https://github.com/django/django/pull/14471