CSRF template tag patch done
Luke Plant
The patch has been added to:
http://code.djangoproject.com/ticket/9977
It includes tests, docs etc - I think it is complete. Other notes are
below (some of this would need to be prominently noted in the release
notes).
I don't know if this is too late for the beta. Since I guess we are
working on US time, and I'm GMT, I'm guessing I may to have to leave
this to one of the other committers. I'll check email before midnight
GMT and hang out on IRC.
Outline
=======
The approach:
- add a template tag to all POST forms. This requires loading the
csrf template tag library, and also requires using a context
processor to get the token.
- use CsrfViewMiddleware to filter out requests that don't have the
token.
All existing contrib apps use RequestContext, which has made the
changes to them minimal (template only, and settings update).
Thanks to bthomas, Rob Hudson for initial patch.
Testing done
============
I've added tests for template tag etc., there was already pretty
exhaustive testing for the middleware.
The middleware is not enabled for the test suite, so shouldn't cause
any complications.
I've manually tested some views in the admin, but I haven't tested all
the contrib apps, because of the time it would take to set up a
harness for testing them. I believe the existing test suite should
have caught any basic rendering errors in the templates due to typos.
The biggest problem might be that some contrib views don't actually
use RequestContext (I think I checked this, but may have missed some).
Other features
==============
When the middleware rejects a request, you can completely customise
the page that is sent back (goodbye to the unfriendly '403 Forbidden'
page), by setting the CSRF_FAILURE_VIEW setting to provide your custom
view.
Upgrading
=========
Unlike what was committed last week, this patch *requires* changes to
project settings in order for the admin (and other contrib apps) not
to break. It could be considered backwards incompatible for this
reason, but it is not an *API* change, just changes to settings.
The following things must be done:
- django.contrib.csrf added to INSTALLED_APPS (for template tag)
- django.contrib.csrf.middleware.CsrfViewMiddleware added to
MIDDLEWARE_CLASSES (or the existing CsrfMiddleware replaced with
CsrfViewMiddleware)
- django.contrib.csrf.context_processors.csrf added to
TEMPLATE_CONTEXT_PROCESSORS
- Any existing apps should be upgraded to use the {% csrf_token %}
method. Alternatively (but not recommended), the existing
CsrfMiddleware or CsrfResponseMiddleware can be used to cover those
apps.
- Obviously (but worth mentioning), if people have overridden
the affected templates for any contrib apps, they may need
updating. This could be quite a bit of work in some cases, I
imagine. I think that the above solution of using
CsrfResponseMiddleware until the updates are done will also
work in this case.
I don't think the above is that complicated, and compared to the
alternatives that have been proposed, the changes are far less
intrusive. But obviously things can go wrong. To see the impact of
various mistakes, I've outlined a number of scenarios below, it's up
to you to decide how serious these things are. But if we don't do it
now, we'll have to either suffer the existing response re-writing or
have the admin vulnerable to CSRF by default for *another* release.
Scenario 1
----------
Developer doesn't bother to read release notes and do any of the above
updates.
Result: admin app and other contrib apps break immediately (i.e.
templates won't render), due to lack of 'django.contrib.csrf' in
INSTALLED_APPS, causing {% load csrf %} to fail.
Scenario 2
----------
Developer adds django.contrib.csrf to INSTALLED_APPS, but doesn't add
the csrf context processor to TEMPLATE_CONTEXT_PROCESSORS.
Result: the admin app (etc.) will break - it will render, but you will
get 403s if you make any POST requests. If DEBUG = True, the
developer will get a warning informing him of the problem.
Scenario 3
----------
As in (2), but the developer has 'CsrfMiddleware' or
'CsrfResponseMiddleware' installed. This might be accidental, or
because the developer needs the response re-writing middleware for
other apps.
Result: admin etc. will work fine. If DEBUG = True, the developer
will get a warning informing him of the problem.
Scenario 4
----------
The developer adds all settings correctly but doesn't remove
CsrfMiddleware/CsrfResponseMiddleware.
This might be accidental, or because the developer needs the response
re-writing middleware for other apps.
Result: admin will work fine. The CSRF token will get inserted twice,
but this is harmless (it's not even invalid HTML).
(We could fix the double insertions and performance hit by adding
@csrf_response_exempt to relevant views in contrib, if we thought it
was worth it).
Scenario 5
----------
The developer installs all settings apart from adding
'CsrfViewMiddleware'.
Result: no crashes, but no CSRF protection.
Scenario 6
----------
Developer updates all settings correctly, but doesn't upgrade any
other apps to use {% csrf_token %}.
Result: Those apps will break -- users will get 403's when trying to
use POST forms.
--------------
Luke
Luke Plant
> Hi all,
>
> The patch has been added to:
>
> http://code.djangoproject.com/ticket/9977
I've bashed on this a lot more, and discovered (and fixed) several
issues (particularly to do with what happens when sessions are first
created). I've added tests for everything, but some more eyes would
be good, and some more people testing it in real situations.
Regards,
Luke
--
Parenthetical remarks (however relevant) are unnecessary
Luke Plant || http://lukeplant.me.uk/
Jacob Kaplan-Moss
I'm sorry it took me so long to review this patch, but I wanted to
make sure I knew what I was talking about first.
What you've done here is admirable, and I agree that the goal of
out-of-the-box CSRF protection is important, but ultimately I can't
get behind committing this.
It's simply far too draconian: if I forget to do all steps needed to
upgrade, all my contrib apps stop working. And then as soon as I *do*
those steps, all *my* apps that use POST stop working. I just can't
see that causing anything other than a huge amount of pain for all but
the most savvy users. Updating every view that handle a POST is a
*lot* of work for even small apps.
An even bigger problem would be for users of third-party reusable
apps: my personal blog is a mere 645 lines of code, but there's over
10,000 lines of third-party apps I'm building on top of. If I want to
upgrade to 1.1 and keep using the admin, I need to wait for all of
those apps to be upgraded to use CSRF protection.
Now think about an app built on top of Pinax, which itself builds on
other reusable apps...
Ultimately I think we just can't do this, *especially* this late in the game.
Unless I'm completely wrong here -- always a possibility! -- I'm going
to reject this for 1.1, and possibly entirely if it can't be made
easier.
Again, I'm sorry I waited so long to look closely into this; the
lateness is entirely my fault, and so not having this feature in 1.1
(in some form) is also entirely my bad as well.
Jacob
Luke Plant
Just got back from being away, would have replied earlier otherwise...
> It's simply far too draconian: if I forget to do all steps needed
> to upgrade, all my contrib apps stop working. And then as soon as I
> *do* those steps, all *my* apps that use POST stop working. I just
> can't see that causing anything other than a huge amount of pain
> for all but the most savvy users. Updating every view that handle a
> POST is a *lot* of work for even small apps.
>
> An even bigger problem would be for users of third-party reusable
> apps: my personal blog is a mere 645 lines of code, but there's
> over 10,000 lines of third-party apps I'm building on top of. If I
> want to upgrade to 1.1 and keep using the admin, I need to wait for
> all of those apps to be upgraded to use CSRF protection.
I don't think it is quite so drastic, there are some more alternatives
here that I think you've missed (my fault for not explaining clearly).
Here are two ways to keep things 'working' with minimal effort (these
are scenarios 5 and 4 in my previous email):
1) Install the CSRF app (update INSTALLED_APPS and
TEMPLATE_CONTEXT_PROCESSORS), but don't install the middleware.
Result: everything still works, but no CSRF protection.
2) Install the CSRF app (update INSTALLED_APPS and
TEMPLATE_CONTEXT_PROCESSORS), and install *both* CsrfViewMiddleware
and CsrfResponseMiddleware.
Result: everything still works, including automatic CSRF protection as
it was in Django 1.0 (and template tag protection for contrib apps as
well). Performance: you get the performance hit of the old post
processing method. Completely harmless side effect: you get double
insertion of the CSRF token in the contrib apps (this works fine, it
isn't even invalid HTML).
It seems to me the only two alternatives you could be proposing are:
1) No recommended CSRF solution.
Result: everything works, but no CSRF protection.
2) Recommend using the Django 1.0 CsrfMiddleware
Result: everything works, including automatic CSRF protection, with
its nastiness.
I don't think that 'my' options 1) and 2) are so much worse than
'your' 1) and 2) (not wishing to sound aggressive there!)
So, as I see it the only disaster is if people expect to upgrade to
Django 1.1 without changing any settings, and don't even do basic
tests (like loading any page in the admin that has a form in it).
Of course, you would still want all apps to be updated eventually.
The evil thing is that once updated for the Django 1.1 way, they would
stop working with Django 1.0, due to the presence of {% load csrf %}
and {% csrf_token %} in their templates. This is actually the
nastiest part of the whole thing IMO, but I think it affects lots of
other areas and is fairly inevitable -- any app which is changed to
use Django 1.1 features will break if you run it on 1.0.
I'm sorry if I didn't explain this properly, it's a bit involved and
easy to forget that other people haven't thought about it for hours!
I realise we would need to document the above very clearly, I'm not
sure where info about upgrading needs to go.
I did go through my own external applications and update them, BTW,
and found the process pretty painless (just "ack -i '<form.*POST'" and
fix the templates). And, as described above, I could have postponed
it just by using CsrfResponseMiddleware instead.
Regards,
Luke
--
"Smoking cures weight problems...eventually..." (Steven Wright)
Luke Plant || http://lukeplant.me.uk/
Luke Plant
> Completely harmless side effect: you get
> double insertion of the CSRF token in the contrib apps (this works
> fine, it isn't even invalid HTML).
In fact, we can even remove this side effect, and the performance hit
of using the CsrfResponseMiddleware where it is not needed, by using
the 'csrf_response_exempt' decorator on the appropriate views. I don't
know if it's worth the pain of having to decorate all those view
functions, although for the admin views I think it's a one-liner if
you put it in the right place (in AdminSite.admin_view() ?)
Regards,
Luke
--
"The first ten million years were the worst. And the second ten
million, they were the worst too. The third ten million, I didn't
enjoy at all. After that I went into a bit of a decline." (Marvin the
paranoid android)
Luke Plant || http://lukeplant.me.uk/
Luke Plant
> An even bigger problem would be for users of third-party reusable
> apps: my personal blog is a mere 645 lines of code, but there's
> over 10,000 lines of third-party apps I'm building on top of. If I
> want to upgrade to 1.1 and keep using the admin, I need to wait for
> all of those apps to be upgraded to use CSRF protection.
>
> Now think about an app built on top of Pinax, which itself builds
> on other reusable apps...
Further to my previous emails, I've now updated everything to be as
smooth as possible. I've attached the documentation to this email,
which now includes an 'Upgrading notes' section. In the case of
someone just upgrading without reading those notes or any release
notes, the admin will fail immediately due to checks in
check_dependencies(). If the developer simply follows the two new
instructions it gives, they will end up with a site that is in
essentially the same situation as it was before -- working CSRF
protection for all apps if CsrfMiddleware was installed, working apps
but no CSRF protection if it was missing.
They can then follow the upgrade notes and fix other applications at
their leisure.
I think this addresses all your concerns. I'm struggling to *think* of
ways this could be easier now (apart from doing nothing, of course).
I've not updated all the apps to use 'csrf_response_exempt', but the
admin views are updated (at least in the recommended configuration,
using admin_view()).
Regards,
Luke
--
"The number you have dialled is imaginary. Please rotate your
telephone by 90 degrees and try again."
Luke Plant || http://lukeplant.me.uk/