CSRFMiddleware needs template tag

[Bob Thomas]

CSRFMiddleware needs a template tag so that a view-only middleware can be used. See discussion at ​http://groups.google.com/group/django-developers/browse_thread/thread/ae525f270ed46933

[Bob Thomas]

Usage:

• Add 'django.contrib.csrf' to INSTALLED_APPS
• Add 'django.contrib.csrf.context_processors.csrf' to TEMPLATE_CONTEXT_PROCESSORS
• Add 'django.contrib.csrf.middleware.CsrfViewMiddleware' to MIDDLEWARE_CLASSES after SesssionMiddleware

In a template:

{% load csrf %}
<form method="post" action=".">
  {% csrf_token %}
  {{ form }}
  <input type="submit" />
</form>

[Luke Plant]

Thanks Bob.

One - the patch will produce invalid HTML if there is more than one form on the page using the tag. This is due to the 'id' attribute -- it was added to help AJAX which needs to get hold of the token somehow. This should not be needed in most cases now (AJAX is automatically excluded), but changing it will break existing apps that relied on the old CsrfMiddleware. So we need to keep it and work out how not to add it.

Two - the tag doesn't do the right thing in the case of there being no session -- I would expect no output.

(There is still a lot to do in terms of docs, tests etc, and the other things I list on the thread you mentioned, but this is a good start, thanks).

[Rob Hudson <treborhudson@…>]

I've done only a few things with patch named '9977.diff':

• Converted \r\n newlines to \n newlines.
• Removed an unused import.
• Added the init.py to the templatetags directory.
• If there is no CSRF token in the context it outputs nothing.
• Removed the id attribute from the input element.

I'll try to work up some docs and tests for the next patch.

Regarding the input tag's id attribute... the way the middleware worked is if it output anything after the first one the id tag was empty. Since the template tag is a new feature (and is opt-in, in a sense), it would make sense to me that this doesn't need to maintain this behavior.

[Bob Thomas]

don't output when token is missing; don't output multiple id fields

[Bob Thomas]

I fixed the same issues as Rob in the latest patch, and it should no longer output the id attribute multiple times. The tag accepts a "noid" argument now, so you can choose which tag contains the id (by excluding it from the other tags).

[Luke Plant]

The 'noid' solution isn't really practical -- because it is manual, it means that templates for forms are not composable. Personally, I would advocate removing the id attribute altogether. The only use case for it is using the token in AJAX calls, but that shouldn't be necessary any longer (see the CSRF documentation).

Removing the id attribute is slightly backwards incompatible, for the case of javascript that was relying on the behaviour of CsrfMiddleware to insert this attribute. However, it was never documented that the CsrfMiddleware would do this, it was just a nice way to help AJAX apps to get around the middleware. It's similar to the way that the admin HTML/CSS has changed - those changes can easily break custom admin templates or Javascript that was layered on top of the admin, but that's tough. People are going to have to manually change stuff anyway to use the templatetag, so they should be aware of the change.

[Bob Thomas]

Replying to lukeplant:

The 'noid' solution isn't really practical -- because it is manual, it means that templates for forms are not composable. Personally, I would advocate removing the id attribute altogether. The only use case for it is using the token in AJAX calls, but that shouldn't be necessary any longer (see the CSRF documentation).

Removing the id attribute is slightly backwards incompatible, for the case of javascript that was relying on the behaviour of CsrfMiddleware to insert this attribute. However, it was never documented that the CsrfMiddleware would do this, it was just a nice way to help AJAX apps to get around the middleware. It's similar to the way that the admin HTML/CSS has changed - those changes can easily break custom admin templates or Javascript that was layered on top of the admin, but that's tough. People are going to have to manually change stuff anyway to use the templatetag, so they should be aware of the change.

So, in your initial comment you said we needed to keep it and figure out a way to not add it multiple times, and now you propose to remove it entirely. I'd really like to help with this, but I am constantly confused over what you think is the correct approach.

[Luke Plant]

Replying to bthomas:

So, in your initial comment you said we needed to keep it and figure out a way to not add it multiple times, and now you propose to remove it entirely. I'd really like to help with this, but I am constantly confused over what you think is the correct approach.

You're right, I changed my mind, I'm sorry for being a pain and causing you work.

I thought about ways to not include it multiple times, and there are ways, but there aren't any nice ways, whereas with the middleware approach it was easy, so I just don't think it is worth it, for the reasons given above.

Cheers, thanks for persevering despite my vagaries.

[Luke Plant]

update docs, contrib apps, refactor, new features

[Luke Plant]

updated for recent svn changes.

[Luke Plant]

Patch updated for recent changes. Trac doesn't like my patch for some reason (doesn't show anything), but it downloads OK.

[Luke Plant]

Another update (fixed a test so that it runs)

[Bob Thomas]

Why still use request.COOKIES[settings.SESSION_COOKIE_NAME] instead of request.session.session_key? My original patch used the latter, because of a possible (security) issue I reported: #9356. There is potential to abuse SECRET_KEY information if the session middleware is not enabled, but CSRF middleware is (an unlikely scenario, though). My original patch used request.session.session_key, so I assume the change was intentional.

The new files in your patch look odd . . . templatetags/csrf.py and context_processors.py look like they were "doubled" somehow.

I was thinking of this since I wrote my initial version . . . it would be nice to allow the template tag to fallback to using context_processors.request if it is enabled (and content_processors.csrf is not). That might even be a better default configuration in global_settings.

I really like the new failure view. There are too many situations where someone has innocently triggered the CSRF protection and been terrified by the response.

[Luke Plant]

I did it that way to avoid a dependency on ordering of the middleware, and to eventually avoid the actual dependency on the middleware. I was hoping to extend the middleware to have a setting CSRF_COOKIE_NAMES which equalled [SESSION_COOKIE_NAME] by default, and thus make the middleware completely independent of SessionMiddleware. Other developers were interested in that as well.

I'm guessing the vulnerability that you are alluding to is that the current middleware can be used to generate and retrieve md5hash(SECRET_KEY + arbitrary user supplied string). If another part of Django does md5hash(SECRET_KEY + user guessable string) and uses it as a secret token (e.g. in email confirmation), the user will be able to generate the token themselves, by-passing the security mechanism.

I hadn't thought about that in this case (though I've worried about this issue in general in the past). As you say, I think it is only an issue if the session middleware is not enabled, otherwise usually (always?) the user will be redirected to a login screen if their session cookie is invalid.

One way to avoid this is to have a separate secret key for the CSRF middleware. In general, this seems like better practice - avoid the possibility of a secret used in one context being abused in another. But that would require another setting being created.

So, I think I agree that we should change this as per your patch. It means we can only use the middleware for the built-in session framework. Any other ideas anyone?

(Thanks for the catch about doubled files, BTW -- I must have messed up with the reverting and patching and so on and applied the patch twice or something.)

[Bob Thomas]

That's exactly what I was referring to. I think it's reasonable for the CSRF middleware to depend on the session middleware, though. So many other common contrib apps depend on it (authentication, etc). If you're going to do your own session handling and authentication, you should be able to do your own CSRF protection, since it's so closely tied in with sessions.

[Luke Plant]

Hmm.

Fixing the tests to use your method will require a bit more work.

But currently, the SessionStore backend does not validate the session_key before it is accessed:

>>> from django.contrib.sessions.backends.db import SessionStore
>>> s = SessionStore("abc")
>>> s.session_key
'abc'

Given this code in SessionMiddleware:

        session_key = request.COOKIES.get(settings.SESSION_COOKIE_NAME, None)
        request.session = engine.SessionStore(session_key)

... I don't see what difference your method makes, unless I'm missing something.

[Bob Thomas]

Hm. In my testing, it made a difference. I think the difference was that in actual use, SessionStore.load() will be called whenever anything in the session is accessed (like the user id). load() will create a new session key if the one given is invalid. You could ensure this happens in the CSRF middleware by accessing SessionStore._session (or any method that uses it, like __contains__, keys(), etc) before checking session_key.

[Luke Plant]

That's the problem -- we don't want the CSRF middleware to create a session! That would be very bad for any view which does need a session. In fact, if there is no session cookie, just accessing session.session_key creates a new session (but it doesn't if there is a session cookie).

So, the alternative is to use:

if request.session.exists(request.COOKIES[SESSION_COOKIE_NAME]):
...

This gives us a database hit (or other backend), but only a read. This is necessary if we are going to validate the session ID.

[Bob Thomas]

Ok, that sounds like a much better solution then.

[Luke Plant]

Hmm, just thought...

An alternative strategy is to deal with your security vulnerability is just to add some 'CSRF salt'. This is added to the secret key along with the session ID to create the hash.

The default value for the CSRF salt is a setting, and most users won't bother to change it, but it doesn't actually matter that all projects may have the same value. All that matters is that the attacker can't use the middleware to generate hashes for (SECRET_KEY + arbitrary string).

So, I'll add that. But I'll also keep the session key validation, because I think it may improve things - the middleware will treat invalid sessions as non-existant, just like the session middleware.

[Luke Plant]

I've discovered a bit of a flaw...

In the docs for CSRF, I wrote this:

5. Ensure that any views which create a new session do not in the same response present a POST form. Often this is achieved by doing a redirect.

The reason for this is as follows: the CSRF
token that is inserted by the csrf_token tag is calculated
on the basis of the ID of an existing session (which is known
from the cookies attached to the request). When a new session
is created, only the outgoing response object is affected,
which the template tag does not have access to, so the template
tag would produce an incorrect token in this case, resulting in
an error when the user tries to submit the form.

The problem is, Django's contrib apps don't do this (not to mention other apps), and fixing them could be hard. The login view for the admin, for instance, just sets a session value (via 'set_test_cookie') which creates a session if one did not already exist. The login form that is displayed therefore needs a CSRF token, but it doesn't get one because the incoming request didn't have a session cookie. With the current state of this patch, you get an error first time you try to log in - it works the second time, because the session cookie has been created then.

The previous method (response post-processing) got around this by checking outgoing cookies and then the incoming session ID, so it worked fine in this situation.

This pushes us back to requiring the middleware to access request.session.session_key. This works because request.session mutates if the developer creates a session (explicitly or implicitly), and so doesn't simply represent the incoming request. Thankfully, I think we can avoid unnecessary creation of sessions using the 'accessed' attribute.

There is still a hole -- if the session key is changed or created after the template is rendered. That will have to go in the docs.

[Luke Plant]

Updated patch

[Luke Plant]

Fixed some issues caused by use of lazy()

[Luke Plant]

Cleaned up tests and docs a bit.

[Luke Plant]

Updated for trunk, plus small enhancement to improve upgrade behaviour

[Luke Plant]

Added new requirements to AdminSite.check_dependencies()

[Luke Plant]

Added docs about upgrading, used csrf_response_exempt in admin, removed duplicated code

[Glenn Maynard]

I'm not a big fan of the acronym (it's rather overused and buzzwordy), but it seems to apply strongly here: inserting the CSRF template tag manually in every form seems like a massive violation of DRY. I can understand the distaste towards postprocessing, but doing it manually like this seems much worse. I'd need a much stronger argument to do it manually than the unexplained recommendation in the documentation.

Does this work cleanly if both the csrf_token tag and CsrfResponseMiddleware are in use? That's an important case, where the user uses the middleware postprocessor, but imports apps that use explicit tagging. Rendering the form field twice probably won't break anything, but it'd be ugly.

I don't think the _make_token salt needs to be configurable; just use a literal string to make the hash distinct from other uses of SECRET_KEY. I'd suggest prefixing the string to the secret rather than appending it.

[Luke Plant]

The argument against post-processing is that it is ugly, it is a performance hit on every page and that it kills the ability to stream responses.

I agree that having to add this every time is a bit of a pain, and I do like the way you can just do "install and forget" with the post-processing method. I made these points myself in this thread: ​http://groups.google.com/group/django-developers/msg/1afbe3a79db00b71 . But the above criticisms are big ones, and no-one has managed to come up with a solution that involves less work than this template tag method. I have converted several applications to use it, and it was very little work -- a regex to find all POST forms is most of the work.

In response to the DRY criticism: one could argue that every time you write <form action="" method="POST"> or {% extend "base.html" %} you are 'repeating' yourself, but the only way to get rid of that kind of thing is to rely on conventions or other implicit behaviour, and at some point we say "explicit is better than implicit". There is a trade-off between repeating yourself and being explicit. Given the other things we are trying to satisfy, putting the compromise at this point seems reasonable. Also, if you forget the template tag, you are at least secure by default, and you will find out pretty quickly that the form does not work.

This method does work cleanly if you use both the tag and the middleware (this is explained in detail in the docs, along with a migration guide). If you obsess about HTML perfection, even though it is invisible to users, then you might be bothered about the field being inserted twice, but I'm not, especially as the result is perfectly valid HTML that works perfectly on all browsers. A bigger concern is the performance hit for pages that don't need the post-processing, and the @csrf_response_exempt decorator will fix that for you (as well as fixing double insertion of the field).

I don't think the _make_token salt needs to be configurable; just use a literal string to make the hash distinct from other uses of SECRET_KEY. I'd suggest prefixing the string to the secret rather than appending it

Agreed, I'll change it to do that.

[Glenn Maynard]

If you obsess about HTML perfection, even though it is invisible to users, then you might be bothered about the field being inserted twice

I don't consider avoiding sloppy data "obsession".

one could argue that every time you write <form action="" method="POST">

This isn't really repetition, because it's actually specifying something: the form's target and type. By contrast, the CSRF field is, with few exceptions, strictly boilerplate. In any case, existing repetition is a weak justification for more.

In any case, here's an alternative approach. Rails provides a helper for generating form tags. It more or less wraps their equivalent of reverse() and generates the <form> and </form> tags. The tag generation itself isn't very interesting, but it's particularly handy for automating things like this. form(action="url", method="POST"); form(view="view.name", method="GET"), etc. I'd probably implement something like this in my templates (I'm not using Django's) if I wanted to have CSRF tags added without postprocessing. Of course, this is supplementary, not a replacement; there's no reason to block this patch for it.

[Luke Plant]

Merged ticket #10816

[Luke Plant]

'Glenn' and 'bthomas' - please let me know how your names should appear in the AUTHORS file for this patch. Thanks for all your hard work, I'm hopeful this will be added as soon as Django 1.1 is out.

[Glenn Maynard]

Glenn Maynard <glenn@…>.

I'll apply and test the patch soon, but I don't see any functional problems on a read-through.

The "some way to get the CSRF token" is a little odd left as it is. I suppose it should say something like this:

If you are using ``CsrfResponseMiddleware`` and your app creates HTML pages and
forms in some unusual way, (e.g.  it sends fragments of HTML in JavaScript
document.write statements) you might bypass the filter that adds the hidden
field to the form.  In this case, form submission will always fail, and the
field must be added explicitly with the template tag or the
:meth:`~django.contrib.csrf.get_token` method.

[Luke Plant]

Updated for r10617 and doc update

[Glenn Maynard]

Fix assertion failure if a request middleware returns a response, causing the view middleware to not be called (test: test_response_middleware_without_view_middleware).

[Luke Plant]

Glenn, a few thoughts after some reflection:

1. Why don't we reject all POST requests that have neither CSRF cookie nor the session cookie (apart from those views that have been specifically exempted of course)? The current patch will let through POST requests that don't have a CSRF cookie, which allows for login CSRF (​http://www.adambarth.com/papers/2008/barth-jackson-mitchell-b.pdf). A simple change and we can secure against login CSRF, and I can't see any disadvantage to this.

2. Why do we actually need to have a unique CSRF cookie if the user has specified a non-default session cookie name? Since the CSRF is now decoupled from the session framework, and is orthogonal to it, I don't see the need for this.

3. I think a CSRF_COOKIE_DOMAIN setting (default None) would be useful. This way, it is possible for a form that appears on one sub-domain with a target on another subdomain to be CSRF enabled and get through the mechanism. I imagine this would be particularly useful if we go with suggestion (1), which could otherwise cause a cross-sub-domain POST request to be rejected.

With these addressed, I think we have an ace patch:

1. Login CSRF is eliminated (hitherto unaddressed)
2. CSRF protection is decoupled from session framework, removing most false positives caused by session key cycling
3. Post-processing is eliminated (at least for all core/contrib apps, you can still use it if you want), removing performance hit and general nastiness

BTW, I've added your changes in last patch, note you need to 'svn add' some new files. We really should use git or something to collaborate on this...

[Luke Plant]

Updated docs mainly.

[Glenn Maynard]

Why don't we reject all POST requests that have neither CSRF cookie nor the session cookie

The rationale was so forms don't break if the user is filtering cookies (eg. Privoxy), and because this was the existing behavior.

This isn't a problem as long as CSRF exemption is being used properly. I suspect it often won't be--minority cases are often overlooked. But, handling login CSRF is probably more important.

Why do we actually need to have a unique CSRF cookie

It probably doesn't matter; if the user has multiple sites on the same domain with access to each others' cookies, they have to trust each other anyway.

There's a somewhat more convincing argument for explicitly including the CSRF form token in that PDF than the ones I've heard here: if it's added automatically, then any form going out of the trusted site will leak the token.

Another thought: is there a specific benefit to hashing the token? The cookies are sensitive data to begin with; if someone can read your cookies, you're already doomed. There's a benefit to eliminating this hash: JavaScript that creates a form dynamically can include the CSRF token client-side simply by reading the cookie. Right now, scripts doesn't have access to the token unless it's exposed manually in some other way.

[Luke Plant]

Replying to Glenn:

Another thought: is there a specific benefit to hashing the token? The cookies are sensitive data to begin with; if someone can read your cookies, you're already doomed. There's a benefit to eliminating this hash: JavaScript that creates a form dynamically can include the CSRF token client-side simply by reading the cookie. Right now, scripts doesn't have access to the token unless it's exposed manually in some other way.

I've pondered this too - I originally added the hashing with a server-side secret to make sure that the form had indeed been generated by the server, and not by anyone else.

Having thought about it, I can come up with at least one scenario where having this hash is useful. In the case of untrusted sub-domains, a page on attacker.example.com could send a header like this Set-Cookie: csrf=myval,domain=.example.com. They would then be able to generate a form targeted at victim.example.com containing <input name="csrf_token" value="myval">, and since the token would match the cookie they set, the CSRF attack would be successful. With the server side hashing of the token, we avoid this. (I believe that attempts by malicious subdomains to get around this by javascript and iframes etc. will fail due to browser policies, just as with cross-domain attacks).

[Glenn Maynard]

Replying to lukeplant:

a page on attacker.example.com could send a header like this Set-Cookie: csrf=myval,domain=.example.com.

But in that case, you can just use a pre-fabricated cookie and token. Just access the site once in advance to get a cookie and token that match up.

Similar problems happen (as described in the PDF you linked) with HTTPS. A MITM attack, using HTTP, could set the CSRF cookie; then, even if your sensitive forms are HTTPS, you could still CSRF into HTTPS using the cookie you MITM'd earlier.

This response is getting complicated, so I'm going to enumerate the attacks being discussed before continuing:

1: A same-domain site sets a known CSRF cookie in order to set a known CSRF token, and uses it to perform generic CSRF.
2: An active attacker does an MITM to establish an HTTP session, creating a known CSRF on a user's system. It then performs a CSRF attack on the user, targetting HTTPS.
3: A same-domain site sets a known CSRF cookie in order to set a known CSRF token, and then uses that to perform login CSRF.

#1: In principle, this could be fixed by keeping a record of all CSRF tokens which were supplied for each session. Reject any CSRF token which has not legitimately been used for a session. This gets complicated; for example, if the session is reset (creating a new session on logout), the existing CSRF token needs to be copied over to the new session. I hate this idea automatically, and I hope there's a better way.

#2: This one is serious in principle. If your site is completely HTTPS, you reasonably expect CSRF protection to be very strong and not subject to MITM attacks, but if the attacker can MITM, he can fool a user into loading an HTTP page to manipulate the CSRF cookie. Probably any fix for #1 would fix this, too.

#3: If the attacker can manipulate cookies like this, preventing login CSRF is very difficult. No form of "lock CSRF token to session" works--the attacker can simply clear the session cookie, since he doesn't care if the user gets logged out when performing login CSRF. The only way I can think of (with the CSRF token approach) is to lock the CSRF token to the user's IP, so an attacker can't use a pre-fabricated CSRF token. This is unacceptable; some users still have rapidly changing IPs. I suspect this attack combination (login CSRF + same domain) is narrow enough that not protecting against it is acceptable, as long as it's documented.

I'm not personally too concerned about same-domain attacks, as long as the weakness is well-documented. There's a simpler way to fix it: get your own domain and stop sharing a domain with people you don't trust. (I don't know all of the issues with cookie domains, though. I know--having just tried it--that I can't set a domain cookie on a TLD, eg. ".com", at least in FF. Can a user on www.ed.ac.uk set a domain cookie for ".ac.uk"?)

The MITM case bothers me, though. One "fix" would be to challenge-response it. One HTTPS request (eg. "checkout") brings up a new form with a new, one-time-use token; and it has another form (eg. "confirm purchase") which submits using the one-time token. Of course, almost every sane online store does this, to prevent duplicating a purchase, and it'd prevent the MITM issue as well. But it's not a general solution, since you don't want to have to double-confirm everything.

[Luke Plant]

When I tried the git django mirrors they weren't working, so I've set up a mercurial branch containing this stuff:

​http://bitbucket.org/spookylukey/django-trunk-lukeplant/

It is a fork of ​http://bitbucket.org/spookylukey/django-trunk and has all the updates we've talked about, apart from removing the hashing of the cookie token (which you've convinced me about - it was useful when we were session dependent, but not now).

With Firefox and Konqueror, you can't set a cookie for ".co.uk", I've tried that, and I presume that protection is in place for the others. (That worries me slightly, because browsers must need to know rules that will presumably need updating. But anyway...)

I can't help thinking that we need more help from browsers/HTTP, as that paper suggests, to really produce a satisfactory solution, especially with regard to HTTPS and MITM. Also, we need to be aware with regards to login CSRF, in Django it will work slightly differently as a session is created before the user logs in, not on the POST request that contains the authentication credentials, which complicates applying that paper directly.

[Glenn Maynard]

What bugs me is that swapping out CSRF cookies (with cross-site cookies or MITM) feels like a regression. Before, if you swapped out the CSRF token, you had to do so by swapping out the whole session, so you're logging the user out. That meant you could do login CSRF this way (which you could do anyway), but nothing else.

Django it will work slightly differently as a session is created before the user logs in, not on the POST request that contains the authentication credentials, which complicates applying that paper directly.

I havn't read the whole thing (like most papers, it's so long the important points are obscured), but the idea is something like this (which is a bit easier to think about now that it's been in the back of my mind for a day):

#1: In CsrfViewMiddleware.process_view, if a session exists and it has a known CSRF token, it must match the real CSRF cookie. If it doesn't, the CSRF cookie may have bee nswapped out; reject the request (and flush the session to recover the user).
#2: In CsrfViewMiddleware.process_response, if there's an existing session, and it doesn't have a stored CSRF value, copy it.

(To be clear: The CSRF cookie remains the authoritative value of the CSRF token. The value in the session, if any, only validates the CSRF cookie, to prevent swaps.)

This would handle session.flush correctly (eg. on login and logout). When auth.logoout flushes the session, the CSRF cookie is untouched. When we get to CVM.process_response, the existing CSRF value will be immediately copied to the new session.

This would handle session expiry correctly. After session expiry, the CSRF cookie remains. If the user then makes a new request which creates a new session, the existing CSRF cookie is then added to the new session.

It would also keep contrib.session optional. If sessions aren't being used, then security against CSRF cookie tampering is lost, but protection for the major cases (remote CSRF) still works.

I think this would protect against both the HTTP/MITM attack and same-domain CSRF (not including login CSRF). Let me know if you see any holes.

[Glenn Maynard]

One more observation: if you can tamper with a user's CSRF cookie, you can tamper with their session cookie too. In other words, if you can do this, you don't need login CSRF at all. Log in a session yourself, and then set the victim's session to the session you already logged in. So, the combination of "MITM/same-site cookies" + "login CSRF" is a moot issue.

Session replacement is still something that would be nice to protect against, but that's a session security issue and irrelevant to CSRF (if we implement session-CSRF-locking as above).

[Luke Plant]

OK, I've had another think about this, and since it is getting rather complicated, I've created a wiki page: CsrfProtection

Let's move any discussion to a 'Talk' section at the bottom of that page. I haven't thought about the proposal you made about the optional use of session, but it seems a bit complex, and I think what I've come up with on that page covers all our bases a bit better.

[Glenn Maynard]

How do I get email notifications when a wiki page is changed (excluding minor changes)? If there's no way to do that, then wiki pages aren't usable for discussion.

Referer checking helps login CSRF, but I don't think it helps same-domain session fixing. I'm not sure any generally acceptable method fixes that yet, though.

My approach protects against same-domain attacks via HTTP; referer checking less reliable for this.

The method I described has a property which makes it a bit simpler: its behavior can nest outside of the main CSRF handling. In fact, it could probably be implemented as a separate middleware (though there's no reason to actually do so). I may implement it as a proof-of-concept to see how tricky it is.

[Luke Plant]

I can't find a way to get notifications. I guess we can discuss on here, but conclusions (so that other people can assess them) should be on the wiki page.

I'm nervous about your proposal because it's just quite complex. It would be easy for there to be some corner that we haven't thought about. The solutions on the wiki page are all well known and well analysed ones (there was the one I implemented for Django 1.0, and the one you produced a patch for in #10816, which are documented exactly in the PDF paper, which nonetheless threw up issues that we hadn't thought about). I'm not that worried about fixing cross subdomain attacks because allowing untrusted subdomains is a serious vulnerability anyway due to session fixation. So, to ease analysis, implementation, documentation, and understanding by the end user, I vote KISS.

[Chris Beaven]

Functionality of #10816 has been merged into this ticket by lukeplant - if this ticket fails design decision then that one should be reopened for review.

[Glenn Maynard]

Refresh my memory: what's the point of _make_token() again? Why isn't the CSRF cookie equal to the CSRF token?

I need this to fill in the CSRF form field from another part of a site I'm developing; it's written in PHP, so it doesn't have access to the Python helpers. I could copy over the secret key and hash it myself, but I can't remember any purpose to this hashing.

I specifically remember discussing the advantages: so your own JS can add the CSRF token for generated forms, which is just another form of what I'm doing. I can't remember why we didn't do it--my guess is we just forgot. I've made this change locally and it's working well.

[Luke Plant]

You're right, _make_token no longer servers a purpose. I just didn't get round to changing it.

[Glenn Maynard]

Updated patch:

• Remove _make_token and use the CSRF cookie directly as the token.
• Don't uniquify the CSRF cookie based on the session cookie name. This was needed when the CSRF token was hashed based on the site's secret key, but since it no longer is, it should be perfectly fine for all sites sharing cookies to share a CSRF token.
• Added settings.CSRF_COOKIE_NAME and CSRF_COOKIE_DOMAIN for the CSRF cookie.
• Updated docs and tests. The previous two cookie settings, SESSION_COOKIE_NAME and LANGUAGE_COOKIE_NAME reference each other explicitly ("should be different from..."), but I didn't update that; having every cookie setting listing every other cookie setting doesn't seem like a good approach. Maybe they should say "different from other COOKIE_NAME settings".
• Tweaked the CSRF view error text; don't accuse hapless users of forgery. (Of course, it's actually accusing some third party website of forgery, but most users wouldn't know that.)

I didn't add a CSRF_COOKIE_PATH setting. The rationale in the SESSION_COOKIE_PATH documentation (having separate sessions in different Django instances) doesn't apply here: it's perfectly OK for unrelated Django instances to pick up and reuse each other's authentication tokens if they're visible. We can add it if someone actually wants it.

[Glenn Maynard]

​http://code.djangoproject.com/ticket/9977#comment:41

[Luke Plant]

Thanks Glenn.

I had already implemented some of this (in particular the CSRF_COOKIE_DOMAIN setting) in my hg repository that I mentioned before: ​http://bitbucket.org/spookylukey/django-trunk-lukeplant/

I've now merged in your changes. I kept the existing name of the CSRF cookie ("csrf_token") rather than use "authid" which seemed a bit obscure.
If possible, 'hg bundles' or pull requests against that repo would be preferred -- I'm using a repos since this patch is rather long-lived and I want easy merges with trunk.

At some point I'll implement the remaining things on the CsrfProtection page, but probably after it has been fully discussed, post the 1.1 release.

Note to other contributors: MOST RECENT PATCH IS NOT STORED ON THIS TICKET, but in hg repository mentioned above.

[Glenn Maynard]

(Sorry, I really don't want to learn a new source control system, and HG's webpage doesn't have a clear explanation of why, as a happy, expert Subversion user, I should even care about it. They seem to expect me to learn how to use it, in order to figure out whether I should bother learning how to use it; it doesn't work that way...)

"csrf_token" just seemed off to me--it's not a token that implements CSRF, it's a token that authorizes form submissions.

I think "csrfmiddlewaretoken" in forms should be renamed to at least "csrftoken". "Middleware" is an implementation detail--there's no need for that to leak into HTML.

[Glenn Maynard]

Another tweak: only set the CSRF cookie if get_token was called.

def get_token(request):
    """
    Returns the the CSRF token required for a POST form, or None if the CSRF middleware
    is not installed.
    """
    request.META["CSRF_COOKIE_USED"] = True
    return request.META.get("CSRF_COOKIE", None)

...
        if not request.META.get("CSRF_COOKIE_USED", False):
            return response

        response.set_cookie(settings.CSRF_COOKIE_NAME,
                request.META["CSRF_COOKIE"], max_age = 60 * 60 * 24 * 7 * 52,
                domain=settings.CSRF_COOKIE_DOMAIN)

This avoids setting the header on every request, which is one of those little incremental bits of waste in every request that Django shouldn't be accumulating.

[Luke Plant]

Hmm, not sure about that. It's a nice tweak, but:

• it makes the tests fail, and the fix isn't entirely trivial.
• if forms are created dynamically client side (e.g. by loading a bit of HTML via AJAX), they will fail since the cookie has not been set.

Also, renaming csrfmiddlewaretoken to csrftoken would be good, but it will give upgrade problems, since it used to be called csrfmiddlewaretoken. At the moment, upgrading will be seamless for users even if they loaded a form before the upgrade and submitted it after, and it would be good to keep that.

With regards to "authid", I just thought it was too generic. "authtoken" might be better, but hHaving "csrf" in it gives a big clue as to where it has come from, and reduces the chance of clashes.

[Glenn Maynard]

Hmm, not sure about that. It's a nice tweak, but:

• it makes the tests fail, and the fix isn't entirely trivial.

Not implementing a feature because it's a pain to update the tests is the worst possible outcome of broad testing. I can do it when I have some time, if we agree it's a good idea and you update the patch on the ticket so I'm not working off old code.

• if forms are created dynamically client side (e.g. by loading a bit of HTML via AJAX), they will fail since the cookie has not been set.

If you load HTML via AJAX, the AJAX response itself sets the cookie.

Also, renaming csrfmiddlewaretoken to csrftoken would be good, but it will give upgrade problems, since it used to be called csrfmiddlewaretoken. At the moment, upgrading will be seamless for users even if they loaded a form before the upgrade and submitted it after, and it would be good to keep that.

Not always; if you have two forms loaded, reloading one will invalidate the other, because it'll set the real CSRF cookie.

[Glenn Maynard]

There's a bigger problem with always sending the header: it means responses are never cachable, since the CSRF cookie is inherently private.

It took me a while to figure out why Apache's mod_cache wasn't caching some views I'm returning that are always intended to be cached--because they had that cookie in there. The same issue would happen with any cache (Squid, etc).

This makes it a serious problem; I'd like to get this fixed.

[Glenn Maynard]

Before someone else knocks me over the head with it, yeah, mod_cache is so buggy it's useless (Vary: handling is totally broken). Time to look for a better cache. The simpler fix for this seems to be Cache-control: no-cache="set-cookie".

Not including the header is still better--cookies are, in fact, cachable on a private cache, and the CSRF middleware obviously shouldn't blindly add that header. So, I'd still like to get this cleaned up.

[Luke Plant]

I still don't like the hidden side effect of get_token(), but you convinced me. I added your code and implemented the tests.

For testing this type of requirement we actually need functional/integration tests, rather than (or in addition to) narrow unit tests. I managed to write tests that passed, but overall it was still broken — because the context processor was not lazy, get_token() was called when {% csrf_token %} was not used.

Updated patch will be attached.

(BTW, that rationale for using something like Mecurial is that it makes this kind of work much, much easier than Subversion + diffs. Subversion completely fails here because: 1) it has very poor merge support 2) we can't develop this kind of thing inside the main repository anyway. Diffing diffs to work out what someone has changed is not much fun, neither is updating the patch against trunk. With Mercurial, updating your branch from trunk is one or two commands, and integrating someone else's work is one command, and it doesn't get harder if lots of people are working on it. But I understand if you don't want to learn to use it).

[Luke Plant]

Updated to trunk, cookie only sent if get_token() used

[Luke Plant]

Implemented strict referer checking for HTTPS

[Luke Plant]

Added PendingDeprecationWarning for CsrfResponseMiddleware

[Luke Plant]

Small doc updates

[Luke Plant]

Mainly updated tutorials, and update to trunk

[Luke Plant]

Helpful 403 page if DEBUG is True

[Luke Plant]

Updated 403 help message

[Luke Plant]

Set 'Vary: Cookie', fixed docs for admin, updated to trunk

[Luke Plant]

lots of updates, including moving functionality to builtin.

[Alex Gaynor]

Fixed in r11660.

[Glenn Maynard]

FYI, the name's Glenn Maynard (glenn@…).

[Jacob]

Milestone 1.2 deleted