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.

Sign up for GitHub

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

Jump to bottom

Fixed #24696 -- Made CSRF_COOKIE computation lazy if not set. #4550

Closed
wants to merge 1 commit into from
Closed

Fixed #24696 -- Made CSRF_COOKIE computation lazy if not set. #4550

wants to merge 1 commit into from

Conversation

electricjay
Copy link
Contributor

Only compute the CSRF_COOKIE when it is actually used. This is a
significant speedup for clients not using cookies.

Changes results of the “test_token_node_no_csrf_cookie” test: It gets
a valid csrf token now which seems like the correct behavior.

This speeds up trivial views by about 40%

@timgraham
Copy link
Member

There's a failing test with this change:

======================================================================
ERROR: test_login_csrf_rotate (auth_tests.test_views.LoginTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/auth_tests/test_views.py", line 613, in test_login_csrf_rotate
    token1 = csrf_cookie.coded_value
AttributeError: 'NoneType' object has no attribute 'coded_value'

@electricjay
Copy link
Contributor Author

The auth_tests.test_views.LoginTest. test_login_csrf_rotate test fails because it is manually manipulating the request.META["CSRF_COOKIE_USED"] variable.

If we need to support the case where apps or other middleware change that variable directly I can add support for that, but if not I can fix the test to not rely on that implementation detail of the csrf middleware.

@timgraham
Copy link
Member

I'd suggest posting on the django-developers list to get some more eyes on this change and advice on that.

@carljm
Copy link
Member

carljm commented Apr 24, 2015

I think this change is correct, but I wouldn't mind confirmation from @PaulMcMillan or @spookylukey or someone else familiar with our CSRF implementation.

The comment starting on (new) line 196 of middleware/csrf.py also needs to be updated for consistency with this change. I think the behavior is still ok (request.META['CSRF_COOKIE'] shouldn't ever be None unless request.META['CSRF_COOKIE_USED'] is also False or not present, in which case we'll bail out at the following check anyway), but the comment is plainly wrong now.

@electricjay
Copy link
Contributor Author

carljm - RE: comment on line 196 of middleware/csrf.py

Since it should not be possible for CSRF_COOKIE_USED to be true and for CSRF_COOKIE to be none at the same time, should we just eliminate the whole comment and check for the CSRF_COOKIE and let the check for CSRF_COOKIE_USED do all the work?

I don't think there are any cases where it is legal to call get_token before CsrfViewMiddleware.process_view has run? If get_token was called first we be generating a new token even if the request supplied one in the cookies.

@carljm
Copy link
Member

carljm commented Apr 24, 2015

Yeah, I think we should just eliminate that check. With your change, any scenario where CSRF_COOKIE_USED but there's no CSRF_COOKIE is a bug in user code that would have to be caused by messing about with internals (because there are only two places that set CSRF_COOKIE_USED, and with your change both of those places also guarantee that CSRF_COOKIE is set). In the case of such a bug in user code, better to fail fast with an error than just silently send no cookie, I think.

carljm
carljm reviewed Apr 24, 2015
View reviewed changes
django/middleware/csrf.py

A side effect of calling this function is to make the csrf_protect
decorator and the CsrfViewMiddleware add a CSRF cookie and a 'Vary: Cookie'
header to the outgoing response. For this reason, you may need to use this
function lazily, as is done by the csrf context processor.
"""
if "CSRF_COOKIE" not in request.META:
request.META["CSRF_COOKIE"] = _get_new_csrf_key()
request.META["CSRF_COOKIE_USED"] = True
return request.META.get("CSRF_COOKIE", None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there's no longer a need for the defensive use of .get() here; CSRF_COOKIE must always be set at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@timgraham timgraham changed the title Lazily compute CSRF_COOKIE if not set. Fixed #24696 -- Made CSRF_COOKIE computation lazy if not set. Apr 25, 2015
@timgraham
Copy link
Member

Could you please squash your commits?

@electricjay
Lazily compute CSRF_COOKIE if not set.
478554c 
Only compute the CSRF_COOKIE when it is actually used.  This is a
significant speedup for clients not using cookies.

Changes results of the “test_token_node_no_csrf_cookie” test:  It gets
a valid csrf token now which seems like the correct behavior.

Changed auth_tests.test_views.LoginTest.test_login_csrf_rotate to
use “get_token” to trigger csrf cookie inclusion instead of changing
request.META["CSRF_COOKIE_USED"] directly.
@timgraham
Copy link
Member

merged in eef95ea, thanks!

@timgraham timgraham closed this May 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@electricjay @timgraham @carljm