CSRF proposal and patch ready for review
Luke Plant
Some big changes to the CSRF protection nearly got in to Django 1.1,
but didn't. Since then, more work has been done, overhauling the
whole thing really. There has been a huge amount of discussion on
mailing lists and tickets, so I've put together what I consider to be
the conclusions, and my proposal for a way forward, on this page:
http://code.djangoproject.com/wiki/CsrfProtection
That page has almost everything you need to know, but just to
highlight some things:
* The proposal is implemented in the lp-csrf_rework branch of
this repo:
http://bitbucket.org/spookylukey/django-trunk-lukeplant/
The difference from trunk is regularly copied to patches on ticket
#9977, if you want simple diffs.
* The proposal (at the bottom of that page) needs review and
consensus.
* Most of the implementation has been pretty well tested, but of
course could do with review, especially as it is security related.
* The strict referer checking for HTTPS has not been tested at all
in any live situations. (I don't have a live HTTPS site handy
for doing such tests). It's possible that special attention also
needs to be paid to HTTPS on non-default ports. Obviously these
things need to be addressed before this could go into trunk.
* The tutorial has not been updated. Fixing it is not trivial.
Since we are including the CsrfViewMiddleware in
project_template/settings.py, POST forms, such as the one in
tutorial 4, require {% load csrf %} and {% csrf_token %}, and
corresponding view functions (created in tutorial 3) require
RequestContext. I don't know what to do about this. Fixing
it is ugly, not fixing it is worse. One option is to rewrite the
view function from tutorial 3 in tutorial 4, combining the two
views in one -- they have a fair amount of redundant code
anyway -- and add RequestContext at that point.
I'm away for 3 weeks from this Wednesday, so I won't be able to
respond to questions during that time. I imagine for most queries
about the patch, 'Glenn' from ticket #9977, who has done a lot of the
work on this (many thanks Glenn!), will be able to answer most things.
Hopefully he's on the list!
It would be best for discussion to take place on this list (or on
ticket #9977), the wiki page is just for gathering conclusions.
Thanks,
Luke
--
"If your parents never had children, the chances are you won't
either."
Luke Plant || http://lukeplant.me.uk/
Luke Plant
I've now addressed everything outstanding (tested under HTTPS and
updated the tutorials), and I've included a friendly summary at the
top of http://code.djangoproject.com/wiki/CsrfProtection
For those wanting to see the patch, for once Trac hasn't barfed on it,
so you can see it with nice formatting here:
http://code.djangoproject.com/attachment/ticket/9977/csrf_template_tag_r11477_1.diff
As far as I'm concerned, this is ready for checkin, except that I
haven't had *any* recent feedback or thumbs up etc. from the list or
other core devs.
This is a breaking change (i.e. there are required changes to some
settings for things to continue to work), so it needs some attention,
and it's security related as well (which justifies the breakage as
well as more attention IMO). I really don't want this to sit around
bitrotting or eventually get postponed to Django 1.3. It's best going
in ASAP, so that we can iron out any problems with the upgrade
instructions before releasing 1.2.
Thanks,
Luke
--
"I'm at peace with the world. I'm completely serene. I know why I was
put here and why everything exists. I am here so everybody can do
what I want. Once everybody accepts it, they'll be serene too."
(Calvin and Hobbes)
Luke Plant || http://lukeplant.me.uk/
Russell Keith-Magee
>
> Can I have some feedback on this please?
> As far as I'm concerned, this is ready for checkin, except that I
> haven't had *any* recent feedback or thumbs up etc. from the list or
> other core devs.
Apologies, Luke. I had this one flagged in my inbox as something to
look at, and then I got distracted by other shiny objects.
I've got some free time right now - I'll try to get back to you ASAP.
Yours,
Russ Magee %-)
Russell Keith-Magee
I've had a quick look at the patch, and found a few minor cosmetic
things. I've also done a lot of reading of the archives to understand
why the patch is the way it is. A comprehensive teardown of the patch
will take a bit longer, but before I do that teardown, I think there's
a bigger issue that we need to address.
Are we really sure we want to do this?
Jacob flagged this back on April 1, and as far as I can make out, his
criticism is still valid. The problem is summed up by the following
question:
Are you absolutely sure that you have correctly updated all the
templates that Django ships?
You say that the update process is a simple search and replace, and
I'm sure you've been very thorough, but every form you've accidentally
missed (or overlooked because it renders the form in a strange way)
will be a regression that throws 403 errors.
Every single Django user will need to undergo the same code and
template update process. Your 'search and replace' estimate also
discounts the the engineering cost of testing and validation that we
will be imposing on every single user.
Another consequence - the external community of apps will also be
suddenly split into two - those that have been updated for CSRF
support, and those that haven't. You won't be able to use a v1.1
Django-compatible app on a Django v1.2 install without disabling CSRF
support.
One option you've mentioned is to install the CSRFResponseMiddleware
as a transition solution. This will hide the problem for v1.2, but it
will just come back to bite use when we complete the deprecation
cycle. At that point, any form that hasn't been updated will stop
working. At which point, we're back to the engineering overhead of the
update.
And all of this is dancing around the fact that the tag insertion
process is just messy. Every form template needs to import the CSRF
module. Every view with a form _must_ use a RequestContext. The
upgrade path for existing apps involves a lot of little changes. Yes,
you've documented the transition, but every extra step in a transition
document is one more thing that can (and will) be misinterpreted or
misunderstood by somebody. There's just too many moving parts involved
here for this to be a painless transition for everyone.
I completely agree that CSRF protection is important, and it's a
crying shame that the CSRF middleware is off by default. As Simon
mentioned early on, this probably means that the CSRF middleware isn't
getting used anywhere near as often as it should.
I also follow the logic that lead from the SafeForm to the template
tag approach.
However, I'm just not convinced that what we're left with is something
we want to impose as a project default. IMHO, the major flaws of the
middleware insertion approach are almost preferable to the
complexities introduced by the tag approach.
A litmus test of this - the tutorial, which is pretty much the
simplest semi-real application you can build in Django, should be CSRF
protected without needing to explain at length what CSRF is. The
template tag solution current fails this test pretty comprehensively.
It's getting late in my corner of the world, and (sin of sins) I
haven't given enough thought to this problem to be able to offer a
counter-proposal at this time. However, I wanted to get you _a_
response given that you've waited so long for feedback.
Yours,
Russ Magee %-)
Luke Plant
> I've had a quick look at the patch, and found a few minor cosmetic
> things. I've also done a lot of reading of the archives to
> understand why the patch is the way it is. A comprehensive teardown
> of the patch will take a bit longer, but before I do that teardown,
> I think there's a bigger issue that we need to address.
>
> Are we really sure we want to do this?
>
> Jacob flagged this back on April 1, and as far as I can make out,
> his criticism is still valid.
I presume you are referring to Jacob's message on Apr 1 on this
thread:
http://groups.google.com/group/django-
developers/browse_thread/thread/c23f556b88cedbc7?pli=1
I replied to his criticisms, which were not all correct at the time,
but didn't receive a response, so I don't think it's fair to say his
criticism is still valid.
> The problem is summed up by the
> following question:
>
> Are you absolutely sure that you have correctly updated all the
> templates that Django ships?
I'm not foolish enough to say "yes" here. I've been as thorough as I
can be, and I checked all the view functions as well as templates.
I've just checked again for code added to Django since I first did
this, and found some example templates in the 'auth' and 'form-wizrd'
docs that needed updating.
But I understand that your question isn't really about how thorough
I've been, it's about the fact that I can't be sure, because the tests
don't cover this, and most people's tests won't cover this. That's
because CsrfViewMiddleware probably won't be enabled for test runs,
and if it is, anything that tests view functions will fail badly.
This, in turn, is because most of our testing of view functions is
over-simplistic -- the tests use a very basic 'HTTP client' to make
requests, rather than something which actually simulates a web browser
(and includes hidden fields that were in the original form).
I can't think of a easy way to fix this. The only proper way is to
migrate *all* tests of view functions to something like twill.
> You say that the update process is a simple search and replace, and
> I'm sure you've been very thorough, but every form you've
> accidentally missed (or overlooked because it renders the form in a
> strange way) will be a regression that throws 403 errors.
>
> Every single Django user will need to undergo the same code and
> template update process. Your 'search and replace' estimate also
> discounts the the engineering cost of testing and validation that
> we will be imposing on every single user.
Yep, there is a significant cost to upgrade, and the "search and
replace" idea is definitely an understatement -- while for contrib
apps and my own apps, a simple 'grep' or 'ack' does suffice to find
all the templates that need fixing, they then need to be traced back
to view functions to check that they are using RequestContext, which
is already true for contrib apps, but fixing view functions that don't
already use RequestContext is a bit more expensive than fixing
templates.
> Another consequence - the external community of apps will also be
> suddenly split into two - those that have been updated for CSRF
> support, and those that haven't. You won't be able to use a v1.1
> Django-compatible app on a Django v1.2 install without disabling
> CSRF support.
That's not really correct - a Django 1.1 app can be run on Django 1.2
by using the CsrfResponseMiddleware (as before), and it will work
fine. What you can't do is use an app that has been updated to use
the 1.2 CSRF template tag on 1.1. I agree that this is a serious
consequence, because once an app is updated for 1.2, it forces users
of that app to upgrade to 1.2. But that is true of any app that uses
a feature only found in 1.2.
> One option you've mentioned is to install the
> CSRFResponseMiddleware as a transition solution. This will hide the
> problem for v1.2, but it will just come back to bite use when we
> complete the deprecation cycle. At that point, any form that hasn't
> been updated will stop working. At which point, we're back to the
> engineering overhead of the update.
I don't actually see your objection here -- isn't this what
'deprecated' means? It means you're being given warning that
something is going to stop working in the future, but you have time to
fix it now. This is true of any feature we deprecate - it will cause
engineering overhead eventually, and the cycle exists so that people
won't simply refuse to upgrade to 1.2.
> And all of this is dancing around the fact that the tag insertion
> process is just messy. Every form template needs to import the CSRF
> module. Every view with a form _must_ use a RequestContext. The
> upgrade path for existing apps involves a lot of little changes.
> Yes, you've documented the transition, but every extra step in a
> transition document is one more thing that can (and will) be
> misinterpreted or misunderstood by somebody. There's just too many
> moving parts involved here for this to be a painless transition for
> everyone.
>
> I completely agree that CSRF protection is important, and it's a
> crying shame that the CSRF middleware is off by default. As Simon
> mentioned early on, this probably means that the CSRF middleware
> isn't getting used anywhere near as often as it should.
>
> I also follow the logic that lead from the SafeForm to the template
> tag approach.
>
> However, I'm just not convinced that what we're left with is
> something we want to impose as a project default. IMHO, the major
> flaws of the middleware insertion approach are almost preferable to
> the complexities introduced by the tag approach.
This is kind of ironic - I pretty much argued this here:
http://groups.google.com/group/django-developers/msg/1afbe3a79db00b71
But I've done my very best to minimize the concerns I listed there,
and I do think that the tag approach is better, despite its flaws.
One major reason is that if you have two POST forms in a page, one
which is external and the other internal (a reasonable possibility on
sites that have a login box on every page), then the automatic tag
insertion method will silently give you a vulnerability, because it
will send the CSRF token to an external site, and it's not possible to
fix it. This is highlighting the fact that control of when the token
is inserted should be on a per-form basis. Given that requirement, I
don't see how you can get round needing something like a template tag
in each form. (Disadvantages of alternatives like SafeForm have been
discussed already).
Even without two POST forms, i.e. if you have just one POST form that
targets an external URL, while you can fix it, you still have to
remember to add the csrf_response_exempt decorator to the view
function. In fact, I've just discovered that there is a view in
current Django that, if you have the current CSRF protection enabled,
will leak the CSRF token to an external site -- the technical 500
debug view in django/views/debug.py has a POST form to dpaste.com.
(I'll try to fix that soon, I don't think dpaste.com is likely to be
malicious). This highlights what happens when you try to
automatically and silently do something that you don't actually have
enough information to do. With the template tag method, you are safe
from this kind of accident.
> A litmus test of this - the tutorial, which is pretty much the
> simplest semi-real application you can build in Django, should be
> CSRF protected without needing to explain at length what CSRF is.
> The template tag solution current fails this test pretty
> comprehensively.
I agree that the tutorials are a litmus test, and it is a big shame
that they are affected. On the other hand, I think the changes I've
made to the tutorials, while probably not exactly polished, are
adequate, and they don't explain at length what CSRF is, and I don't
think they need to. The updated tutorial does highlight the problem
of CSRF, and it's a problem that unfortunately every web developer
needs to know *something* about. It's becoming more exploited in the
real world, and just like developers need to know *something* about
XSS and SQL injection, they do need to know about CSRF.
IMO, the only time when it's justified to completely hide an issue
like this from developers is when we can guarantee that it won't be a
problem. To use examples from Django, the docs mention nothing about
SQL-injection, because the ORM and the API for raw SQL (via
cursor.execute) do not open up vulnerabilities. The template docs
*do* talk about XSS under the 'Automatic HTML escaping' section,
because that is a bit of 'magic' that the developer may need control
over, and so will need to know about. For CSRF, we can't
automatically fix it, so we can't pretend it doesn't exist.
Thanks again for your feedback. Like you, I'm far from being 100%
happy with this solution, but it's the lesser of two evils IMO.
Regards,
Luke
--
"Ineptitude: If you can't learn to do something well, learn to enjoy
doing it poorly." (despair.com)
Luke Plant || http://lukeplant.me.uk/
Luke Plant
> In fact, I've just discovered that there is a view in
> current Django that, if you have the current CSRF protection
> enabled, will leak the CSRF token to an external site -- the
> technical 500 debug view in django/views/debug.py has a POST form
> to dpaste.com. (I'll try to fix that soon, I don't think dpaste.com
> is likely to be malicious).
Actually, I've realised I would need to import a contrib function
(csrf_response_exempt) into core to fix this. That's a bad thing to
do, so I'm thinking of not fixing this unless anyone screams.
Luke
--
I never hated a man enough to give him his diamonds back. (Zsa Zsa
Gabor)
Luke Plant || http://lukeplant.me.uk/
Russell Keith-Magee
>
> Thanks for your response Russell:
>
>> I've had a quick look at the patch, and found a few minor cosmetic
>> things. I've also done a lot of reading of the archives to
>> understand why the patch is the way it is. A comprehensive teardown
>> of the patch will take a bit longer, but before I do that teardown,
>> I think there's a bigger issue that we need to address.
>>
>> Are we really sure we want to do this?
>>
>> Jacob flagged this back on April 1, and as far as I can make out,
>> his criticism is still valid.
>
> I presume you are referring to Jacob's message on Apr 1 on this
> thread:
>
> http://groups.google.com/group/django-
> developers/browse_thread/thread/c23f556b88cedbc7?pli=1
That's the one.
> I replied to his criticisms, which were not all correct at the time,
> but didn't receive a response, so I don't think it's fair to say his
> criticism is still valid.
I won't presume to speak for Jacob, but my reading is that his core
complaint - that the proposal as it stands is messy and will require a
serious upgrade effort for every single Django user - is still
accurate. True, his comments had a particular focus because of the
v1.1 deadline, and some of the details about exactly what would be
broken and how it would break are a little off, but I don't see that
you've addressed his core complaint.
>> The problem is summed up by the
>> following question:
>>
>> Are you absolutely sure that you have correctly updated all the
>> templates that Django ships?
>
> I'm not foolish enough to say "yes" here. I've been as thorough as I
> can be, and I checked all the view functions as well as templates.
...
> I can't think of a easy way to fix this. The only proper way is to
> migrate *all* tests of view functions to something like twill.
I'm not sure this is something we _can_ fix. Predicating a change on
the assumption that the community has comprehensive test suites for
their code is... optimistic at best.
>> Another consequence - the external community of apps will also be
>> suddenly split into two - those that have been updated for CSRF
>> support, and those that haven't. You won't be able to use a v1.1
>> Django-compatible app on a Django v1.2 install without disabling
>> CSRF support.
>
> That's not really correct - a Django 1.1 app can be run on Django 1.2
> by using the CsrfResponseMiddleware (as before), and it will work
> fine. What you can't do is use an app that has been updated to use
> the 1.2 CSRF template tag on 1.1. I agree that this is a serious
> consequence, because once an app is updated for 1.2, it forces users
> of that app to upgrade to 1.2. But that is true of any app that uses
> a feature only found in 1.2.
The difference here is the size and scope of the change.
Consider a recent feature addition - aggregation.
* Some projects will adopt the feature straight away and use it
throughout their code. These projects need to put up a prominent "1.1
only" sign.
* Some projects will adopt it in certain locations in their codebase.
For example, they might add a 'summary' view that can be deployed. In
this case, you can continue to use the entire app with Django 1.0 as
long as you don't use the summary view.
* Some apps will never have a use case for aggregation. These
projects will continue to work with Django 1.0 as long as they don't
use the new feature.
In the case of CSRF, the change is to Forms. Not some small or obscure
feature of forms, but the fundamental way that forms are handled.
There won't be too many Django plugabble apps that don't have a form
in them, and every single one of them will fall into category 1 - the
"1.2 only" category. Ok, this is mitigated by the
CSRFResponseMiddleware workaround, but that just defers the problem to
v1.4.
My point is that this is a _big_ change. It's the sort of change that
I might expect to see in a 1.X->2.0 transition, not in a minor point
release.
>> One option you've mentioned is to install the
>> CSRFResponseMiddleware as a transition solution. This will hide the
>> problem for v1.2, but it will just come back to bite use when we
>> complete the deprecation cycle. At that point, any form that hasn't
>> been updated will stop working. At which point, we're back to the
>> engineering overhead of the update.
>
> I don't actually see your objection here -- isn't this what
> 'deprecated' means? It means you're being given warning that
> something is going to stop working in the future, but you have time to
> fix it now. This is true of any feature we deprecate - it will cause
> engineering overhead eventually, and the cycle exists so that people
> won't simply refuse to upgrade to 1.2.
True, but we aren't deprecating some obscure function on the forms
framework here and replacing it with a slightly different definition
that requires a 1 line fix. We're effectively deprecating the way you
write forms.
>> However, I'm just not convinced that what we're left with is
>> something we want to impose as a project default. IMHO, the major
>> flaws of the middleware insertion approach are almost preferable to
>> the complexities introduced by the tag approach.
>
> This is kind of ironic - I pretty much argued this here:
> http://groups.google.com/group/django-developers/msg/1afbe3a79db00b71
Heh - ironic is the word. That message completely sums up my position
- in particular the last comment: I'm not saying don't replace it, but
let's make sure the cure is better than the illness.
> IMO, the only time when it's justified to completely hide an issue
> like this from developers is when we can guarantee that it won't be a
> problem. To use examples from Django, the docs mention nothing about
> SQL-injection, because the ORM and the API for raw SQL (via
> cursor.execute) do not open up vulnerabilities. The template docs
> *do* talk about XSS under the 'Automatic HTML escaping' section,
> because that is a bit of 'magic' that the developer may need control
> over, and so will need to know about. For CSRF, we can't
> automatically fix it, so we can't pretend it doesn't exist.
The difference here is that XSS is mentioned in the template docs, not
the tutorial. The tutorial is happily XSS safe, and the new user is
oblivious to this fact. You only really need to hunt down
documentation about XSS when you start dealing with content that needs
to break the default XSS handling.
CSRF, on the other hand, requires an active effort from the start. If
you start your first project, write a form, and omit the template
tag, or the tag loader, or the middleware, or the template content
processor, or don't use a RequestContext, then everything falls in a
heap of 403s for not entirely obvious (or easy to debug) reasons.
> Thanks again for your feedback. Like you, I'm far from being 100%
> happy with this solution, but it's the lesser of two evils IMO.
I suppose my point is that if you're not 100% happy, and I'm not even
close to 100% happy, and (reading between the lines of old comments)
Jacob isn't happy, then we still have some work to do. :-)
I haven't had a chance to get my thoughts together yet - I've been
mostly ignoring the CSRF debate up to this point, so I'm still coming
up to speed with what has already been tried. My protoplasmic thoughts
at the moment are mostly in the vein of:
1. Is there anything we can do to fix the problems with the
ResponseMiddleware? I know there are problems, but at the core there
is something really good - it seems a waste to throw it all away.
2. Is there anything we can do with a hybrid approach? As I see it,
SafeForm was abandoned because it didn't cover the use case of
hand-crafted forms, but that argument ignores the fact that a good
proportion of Django forms aren't hand generated. A SafeForm where {{
form }} and {{ form.csrf_token }} can be used to include the CSRF
token avoids the need of RequestContext, loading a tag library, or the
CSRF context processor, but it doesn't preclude the existence of a {%
csrf_token %} tag as well for those cases that need the manual
control.
3. CSRF is currently a contrib app. Why? CSRF control is the very
model of a feature that shouldn't be decoupled from the base
framework. If we're aiming to make CSRF support like XSS support,
surely it should be baked into the core, not kept isolated as a
contrib app. Is there anything else that gets easier if we sink this
into the core? At the very least, the tag loading issues go away - are
there other possible benefits?
However, as I said, these ideas are mere zygotes of real thoughts.
I'll let you know when they've gestated a little more.
By any chance, are you coming to DjangoCon? This would be a fantastic
subject for a sprint discussion.
Russ %-)
Luke Plant
> The difference here is that XSS is mentioned in the template docs,
> not the tutorial. The tutorial is happily XSS safe, and the new
> user is oblivious to this fact. You only really need to hunt down
> documentation about XSS when you start dealing with content that
> needs to break the default XSS handling.
>
> CSRF, on the other hand, requires an active effort from the start.
> If you start your first project, write a form, and omit the
> template tag, or the tag loader, or the middleware, or the template
> content processor, or don't use a RequestContext, then everything
> falls in a heap of 403s for not entirely obvious (or easy to debug)
> reasons.
I know you're simplifying, but to qualify slightly:
- if you only omit the middleware (by manually disabling it if you
are following the tutorial, or by not following upgrade
instructions for everyone else), everything works except you have
no actual CSRF protection.
- if you only fail to use the CSRF context processor or
RequestContext, you actually get a helpful warning on the
development server terminal.
- if you fail to use the tag loader, you'll get:
TemplateSyntaxError at /foo/
Invalid block tag: 'csrf_token'
which is better than an inscrutable 403.
So it's not *quite* as bad as you say.
Also, we could improve the default CSRF_FAILURE_VIEW so that if DEBUG
is on, on the 403 page the developer gets a link to the CSRF docs or
something, or a check list of things they might have forgotten. That
would improve things a lot IMO. In fact, I like that a lot and I've
implemented it now, ticket updated.
> 1. Is there anything we can do to fix the problems with the
> ResponseMiddleware? I know there are problems, but at the core
> there is something really good - it seems a waste to throw it all
> away.
If you could make it so that it only inserted the token to the correct
forms, you could perhaps salvage it. The difficulty is that you don't
reliably know what is an external site, AFAICS. You can't just match
the 'action' attribute against the current domain, because the form
might be targeting a different subdomain (which is allowed if you use
the new CSRF_COOKIE_DOMAIN setting). You could possibly use that
setting to help with matching...but I'm concerned that you might not
be able to get it right all the time, and for the exceptions you have
no way of giving the developer the necessary control.
It would still suffer from the fact that you wouldn't be able to do
streaming responses.
> 2. Is there anything we can do with a hybrid approach? As I see
> it, SafeForm was abandoned because it didn't cover the use case of
> hand-crafted forms, but that argument ignores the fact that a good
> proportion of Django forms aren't hand generated. A SafeForm where
> {{ form }} and {{ form.csrf_token }} can be used to include the
> CSRF token avoids the need of RequestContext, loading a tag
> library, or the CSRF context processor, but it doesn't preclude the
> existence of a {% csrf_token %} tag as well for those cases that
> need the manual control.
The APIs for SafeForm that have been proposed so far were fairly bad,
IMO, and significantly worse than the requirement of using
RequestContext. Then you've got the problem of convincing people to
use SafeForm. If you make all built-in forms inherit SafeForm, then
you need a mechanism for turning off the token. It seems to bring
more problems than it solves, and it would be a much bigger change to
the fundamental way we do forms, because Django Forms until now have
not included the <form> tag, which I think the SafeForm solution needs
to do to provide any real benefit. The changes to the tutorial would
be *much* bigger. On top of this, there is the "preferably only one
way to do it" maxim.
> 3. CSRF is currently a contrib app. Why? CSRF control is the very
> model of a feature that shouldn't be decoupled from the base
> framework. If we're aiming to make CSRF support like XSS support,
> surely it should be baked into the core, not kept isolated as a
> contrib app. Is there anything else that gets easier if we sink
> this into the core? At the very least, the tag loading issues go
> away - are there other possible benefits?
I did think about this occasionally. The only other benefit I can see
is if you could eliminate the need for RequestContext. If the
middleware stuck the CSRF token in some thread local storage, or
something evil like that, you could do it, and you would be down to
simply inserting the {% csrf_token %} in every form that needed it,
plus turning the middleware on. It's a significant improvement in
API, (and a relatively small change to the patch, I think), but fairly
evil in other ways. Having additional request-related things
accessible to the templates is exactly the reason RequestContext
exists, so it seems strange to use another mechanism.
> However, as I said, these ideas are mere zygotes of real thoughts.
> I'll let you know when they've gestated a little more.
I replied to them anyway, hopefully to some benefit.
> By any chance, are you coming to DjangoCon? This would be a
> fantastic subject for a sprint discussion.
No, I'm afraid not (though I'd love to go). I have a full time job
which is not programming related, and it would be too far for me
anyway (I'm in the UK).
Luke Plant
> 1. Is there anything we can do to fix the problems with the
> ResponseMiddleware? I know there are problems, but at the core there
> is something really good - it seems a waste to throw it all away.
I just stumbled across a new argument against the CsrfResponseMiddleware:
http://code.djangoproject.com/ticket/9163
To me, this confirms my gut instinct (or rather, Jacob's), that post-
processing the response is really nasty.
Luke
--
Life is complex. It has both real and imaginary components.
Luke Plant || http://lukeplant.me.uk/
Luke Plant
> 3. CSRF is currently a contrib app. Why? CSRF control is the very
> model of a feature that shouldn't be decoupled from the base
> framework. If we're aiming to make CSRF support like XSS support,
> surely it should be baked into the core, not kept isolated as a
> contrib app. Is there anything else that gets easier if we sink this
> into the core? At the very least, the tag loading issues go away - are
> there other possible benefits?
For the sake of recording my thoughts, one advantage of keeping it as a
contrib app is that developers can completely replace the CSRF mechanism if
they don't like the bundled one. Simply by doing
s/django.contrib.csrf/thirdparty.csrf/ to their INSTALLED_APPS,
MIDDLEWARE_CLASSES and TEMPLATE_CONTEXT_PROCESSROS settings,
they would replace the CSRF mechanism (including the templatetag library) with
their own. Their own {% csrf_token %} might return an empty string (e.g. if
they were just using Referer checking or Origin checking or something), but
that's fine. The admin and all other apps would then seamlessly use the
different CSRF mechanism.
So keeping it as contrib and not in core might be an advantage if some
websites have special requirements, or the bundled CSRF mechanism becomes
outdated.
Luke
--
"Mistakes: It could be that the purpose of your life is only to serve
as a warning to others." (despair.com)
Luke Plant || http://lukeplant.me.uk/
Russell Keith-Magee
>
> On Monday 31 August 2009 15:26:42 Russell Keith-Magee wrote:
>
>> 3. CSRF is currently a contrib app. Why? CSRF control is the very
>> model of a feature that shouldn't be decoupled from the base
>> framework. If we're aiming to make CSRF support like XSS support,
>> surely it should be baked into the core, not kept isolated as a
>> contrib app. Is there anything else that gets easier if we sink this
>> into the core? At the very least, the tag loading issues go away - are
>> there other possible benefits?
>
> For the sake of recording my thoughts, one advantage of keeping it as a
> contrib app is that developers can completely replace the CSRF mechanism if
> they don't like the bundled one. Simply by doing
> s/django.contrib.csrf/thirdparty.csrf/ to their INSTALLED_APPS,
> MIDDLEWARE_CLASSES and TEMPLATE_CONTEXT_PROCESSROS settings,
> they would replace the CSRF mechanism (including the templatetag library) with
> their own. Their own {% csrf_token %} might return an empty string (e.g. if
> they were just using Referer checking or Origin checking or something), but
> that's fine. The admin and all other apps would then seamlessly use the
> different CSRF mechanism.
>
> So keeping it as contrib and not in core might be an advantage if some
> websites have special requirements, or the bundled CSRF mechanism becomes
> outdated.
FYI, Simon, Andrew Godwin and myself had a chance to chat about CSRF
during the DjangoCon sprints. I also discussed the issue with a few
other people over the conference.
The CSRF tag approach you have implemented didn't win a lot of fans
whenever I described it, and for pretty much the same reasons I have
expressed previously - too many moving parts, and a little too much
manual intervention required.
The discussions I had with Simon and Andrew revolved around trying to
improve the interface to SafeForm, and I think we may have a workable
solution. I've only just arrived home, so I'm fairly jet lagged at the
moment; I'll try to put the result of our discussions into the form of
a formal proposal over the next day or so.
Yours,
Russ %-)
Luke Plant
> The CSRF tag approach you have implemented didn't win a lot of fans
> whenever I described it, and for pretty much the same reasons I have
> expressed previously - too many moving parts, and a little too much
> manual intervention required.
>
> The discussions I had with Simon and Andrew revolved around trying to
> improve the interface to SafeForm, and I think we may have a workable
> solution. I've only just arrived home, so I'm fairly jet lagged at the
> moment; I'll try to put the result of our discussions into the form of
> a formal proposal over the next day or so.
OK, I'll wait and see.
To clear things up, the template tag approach really has just two 'moving
parts':
- use RequestContext in your view (often you will be covered already e.g.
generic views, and lots of people already using RequestContext everywhere)
- use the csrf tag in the template.
Everything else is global settings i.e. a one time change, which you will be
prompted about *once* if you forget them, and then you can forget them.
I'd be very surprised if SafeForm was a better result - it has major flaws
that can't be addressed by API changes, AFAIC:
* it requires using Django's form system. I've got plenty of views that
don't (e.g. anything with a dynamic number of controls). People not using
django.Forms are left out in the cold, or having to learn another way to do
things.
* it's off by default. Turning it on by default will require making
django.forms.Form subclass SafeForm, which will almost certainly require a
huge and immediate upgrade effort, because SafeForm cannot have the same API
as Form. If it's off by default, I see no point at all in this entire
exercise. If we don't arrive at a point where the POST form created in the
tutorial is safe from CSRF, I think we've failed.
And it will still require changes to templates, just like the template tag,
and it will also require changes to views, only significantly more complex
than simply using RequestContext. It's got at least as many and at least as
intrusive 'moving parts' AFAICS.
It also has the disadvantage that it is much more intrusive - you can't just
switch out the implementation of your CSRF protection mechanism like you can
with the template tag and middleware.
But I'll wait 'til I see your proposal.
Luke
--
"My capacity for happiness you could fit into a matchbox without
taking out the matches first." (Marvin the paranoid android)
Luke Plant || http://lukeplant.me.uk/
Simon Willison
> * it requires using Django's form system. I've got plenty of views that
> don't (e.g. anything with a dynamic number of controls). People not using
> django.Forms are left out in the cold, or having to learn another way to do
> things.
to the CSRF token creation and validation routines (in csrf_utils) -
see the readme for documentation on this.
> * it's off by default. Turning it on by default will require making
> django.forms.Form subclass SafeForm, which will almost certainly require a
> huge and immediate upgrade effort, because SafeForm cannot have the same API
> as Form. If it's off by default, I see no point at all in this entire
> exercise. If we don't arrive at a point where the POST form created in the
> tutorial is safe from CSRF, I think we've failed.
to see all Django applications secure against CSRF by default, I can't
see a way of doing it that doesn't break existing apps. More important
for me though is that the Django admin (and other reusable apps, such
as Pinax stuff) MUST be secure against CSRF out of the box, no matter
what the user puts in their settings.py file. If developers fail to
protect themselves (especially when the solution is well documented)
then at least it isn't our fault.
I think this is subtly different from the XSS escaping issue simply
because the solution to CSRF is much more intrusive.
> And it will still require changes to templates, just like the template tag,
> and it will also require changes to views, only significantly more complex
> than simply using RequestContext. It's got at least as many and at least as
> intrusive 'moving parts' AFAICS.
def change_password(request):
form = ChangePasswordForm()
if request.method == 'POST':
form = ChangePasswordForm(request.POST)
if form.is_valid():
return HttpResponse('Thank you')
return render_to_response('change_password.html', {
'form': form,
})
To this:
@csrf_protect
def change_password(request):
form = ChangePasswordForm(request)
if form.is_valid():
return HttpResponse('Thank you')
return render_to_response('change_password.html', {
'form': form,
})
The SafeForm deals with the request.method == 'POST' bit, meaning one
less conditional branch within the view function itself.
I'm pretty happy with the SafeForm interface now that I've fleshed it
out - it's a lot less clunky than I was originally expecting.
Cheers,
Simon
Russell Keith-Magee
Did we discuss this bit at the sprint? I'm completely on board with
everything else, but this bit makes me nervous:
* It calls is_valid() on the form during a GET (which isn't currently
done as part of normal practice).
* It isn't possible to have any special view handling based on the
request method.
* It implies that the method of construction the form will always be
the same, regardless of whether we are GETting or POSTing. This isn't
always true - in particular, I'm thinking of the construction of
FormSets, which can use a queryset to populate the forms on GET, but
don't require the queryset on POST.
> I'm pretty happy with the SafeForm interface now that I've fleshed it
> out - it's a lot less clunky than I was originally expecting.
Another advantage that Simon hasn't mentioned - it requires no
modifications to the tutorial. We can present the basic ideas of form
handling in tutorial 4 without mentioning CSRF. We then have scope to
introduce a new tutorial on security issues, that can both educate on
the nature of XSS and CSRF attacks, plus what you have to do in order
to protect against them.
Yours,
Russ Magee %-)
Simon Willison
of the CSRF work (since the form needs access to the request object in
both cases) but you've convinced me that it's a step too far - I'll
see if I can fix that.
Cheers,
Simon
On Sep 17, 2:10 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:
Luke Plant
OK, here is my response. I hope this doesn't turn into a personal my-code-vs-
your-code match (especially as most of "my code" is really other people's code
and ideas :-) but I want to make sure we do this right, as it potentially has
big implications, so the following will be "sleeves rolled up" (getting into
details) and "gloves off" when it comes to criticisms of the code itself :-)
Advantages of SafeForm
----------------------
I can see the value of having contrib apps secured whatever the user has in
their settings.py
Unfortunately, that is the only advantage of any significance that I can
see. And there are *much* easier ways to achieve that goal if that is our
main priority (see the bottom of my e-mail for details).
There is the 'advantage' that Russell mentioned of not having to touch the
tutorials. This is a pretty dubious advantage IMO because it means that the
tutorials will still lead to insecure code. Far from 'raising the profile' of
CSRF as Simon rightly suggested we should do, this is just keeping quiet about
it.
Disadvantages of SafeForm
-------------------------
1) Big changes are required to use it.
The changes to use SafeForm are much more intrusive than using the template
tag. It requires quite extensive changes to both templates and views:
* different API and different flow control for SafeForm compared to Form (it
may be better, but it's different, which means a big upgrade cost)
* a decorator for views
* a decorator for forms
* quite commonly, significant changes to the template:
* Often you have to explicitly add the csrf token field. And you've
got to worry about whether you need to or not.
* If you weren't displaying form.non_field_errors before (which can be
common if you don't need it), you have to add it.
* For the multiple forms in a <form> case, you've got even more changes
to your view function and your template, and you have to learn about
CsrfForm on top of everything else.
Using SafeForm is a *lot* more complex than the template tag method. The
basic instructions for day-to-day usage of the template tag method (i.e. not
including change-once settings) go like this:
If the target of a <form> is internal:
* add {% load csrf %} to the template and {% csrf_token %} to the form
* use RequestContext in the corresponding view.
And that's it.
2) More than one way to do CSRF protection.
Although you provide a low level mechanism for forms, it still means you have
Two Ways To Do CSRF Protection, and they are quite different (from the users
point of view). Once you've added in the multiple forms in a <form> problem,
you've now got Three Ways To Do CSRF Protection, all of which will be needed
in quite common circumstances.
(Yes, the template tag also provides a lower level mechanism (using get_token
directly), but developers will only ever need to use it if they not using
templates to generate pages, which is very uncommon).
3) More than one way to do Forms
Switching between Form and SafeForm requires API changes for the form itself
and significant flow control changes in the view. So we have "Two Ways To
Do Forms".
Every user now faces a choice - use Form or SafeForm? Form seems to be
simpler; the API is possible to play with at the interactive prompt (see
tests/regressiontests/forms.py, for example), unlike SafeForm which requires
an HttpRequest object; and SafeForm sounds like something for the paranoid,
ultra-secure types. So you can bet that tutorials and code across the
internet are going to use Form.
Even if people standardise on using SafeForm everywhere, you will have the
case where they use Django forms to generate a form even when it is targeted
externally. Because they have switched to using SafeForm, that form will now
include the CSRF token, which is leaked to external sites, which is a security
vulnerability. Further, it's really not obvious, because the place where the
use of SafeForm is defined (in their forms.py or views.py somewhere) is a very
long way from the information that defines whether they need it or not (the
target attribute of the <form> in the template).
4) CSRF is baked in to view functions.
Every view function now has CSRF baked in, either 'hand rolled' or via
SafeForm. While this is the major advantage of SafeForm, it is also a major
disadvantage, because:
* If we need to change anything about the API in the future, we will require
extensive changes to Django code and everyone else's.
* Developers can't 'swap out' the implementation of CSRF protection. With
the template tag method, this is easy to do. If someone has custom
requirements, or because of their situation they can just do
strict Referer checking or Origin checking or something, they can implement
their own middleware and template tag, and switch *all* the Django apps
over to their system simply by changing some settings.
* Testing view functions becomes a lot harder. Many of the admin view tests
would break if we switched to SafeForm, because you've now got to include
the token, which you have to get from the previous page. So you have to
correctly parse the form (ensuring you don't pick up some other form that
happens to be in the template) and extract the token. Not only do you have
to do this for all the apps in contrib, you are requiring every user of
Django to do this as well - if they want CSRF protection. Testing tools
like twill can solve this problem - once you've rewritten all your tests
of course.
This is a massive disadvantage IMO.
* If developers want to change the way the 'Form session expired' error is
handled, they have to change code across the code base. With the
middleware, they can just implement their own view function and
set CSRF_FAILURE_VIEW. There doesn't really seem to be any need
to handle this on a per-form basis, especially as the false positives
associated with session cycling have now been eliminated - it will be
very rare that a user will ever actually see the CSRF error message.
* It's just messy. We've got flow control and code that's concerned with
one particular security problem spread throughout the code base. This
isn't exactly 'separation of concerns'. The template tag method is not
ideal in this regard, but the SafeForm method is quite a lot worse.
5) Silently insecure
With the middleware, if you forget to add protection to one template/view, the
system will complain loudly and offensively the minute anyone tries to POST to
that view. You can make it nicer if you want by implementing your own
CSRF_FAILURE_VIEW, which could e-mail the admins if the token was absent but
other indicators (like Referer) checked out OK. We could even put that in by
default. But the big point is that you (or someone) will *know* about it, and
you *won't* have a CSRF vulnerability.
With SafeForm, you have to remember to implement it for every POST form. If
you forget one, you have a vulnerability, and you *won't know* until it is
exploited. With lots of Django code being open source re-usable apps,
targetted exploits that rely on knowing the source code are quite feasible.
6) Backwards incompatibilities with contrib Forms.
What do we do with SetPasswordForm etc.? Really, we ought to do this:
SetPasswordForm = SafeForm(SetPasswordForm)
which would of course break everyone's code if they have used or subclassed
these forms. So instead we need:
SafeSetPasswordForm = SafeForm(SetPasswordForm)
Also, we need to take into consideration the various 'extension points' in
contrib code that allow custom forms to be passed in e.g:
* def password_reset_confirm(request, uidb36=None, token=None,
template_name='registration/password_reset_confirm.html',
token_generator=default_token_generator,
set_password_form=SetPasswordForm,
post_reset_redirect=None):
* ModelAdmin.form
* etc.
All of these will have to continue to use the normal Form, because they are
public interfaces and SafeForms are not API compatible. At the last minute,
view code must wrap those forms with SafeForm and then use them. This has the
following consequences:
1. People are going to have to keep and pass around the undecorated Forms, as
well as (sometimes) create Safe versions for their own use. This is pretty
messy.
2. We've taken away some control over the actual form used, since we always
wrap in SafeForm. (If we give the option of using SafeForm or not, things get
*really* messy)
3. If anyone has overridden ModelAdmin.change_view or add_view, they will have
to update it to add the SafeForm decoration, and also add the changes to the
usage of the form instance.
4. There are problems for anyone who has created custom templates. This is
pretty common, especially with the admin. Contrib apps are now going to use
the SafeForm-wrapped-Form with a template that was designed for Form, and
quite often the custom template will be broken (i.e. if it needs to include
the CSRF field or non_field_errors but doesn't). Basically, in order to
upgrade contrib apps to use SafeForm, we also need to check and upgrade all
the templates, but we don't have access to all the templates!
Now, with the template tag, we also need to upgrade custom contrib templates
but:
1) Developers don't have to do it straight away - they can install
CsrfResponseMiddleware and everything will work perfectly.
2) The upgrade is much simpler.
With SafeForm, their templates and views will break instantly (in some cases),
but they won't do so loudly, or with an obvious cause of the error, or with a
workaround to buy them time to update their code.
OK, now onto a reply, an alternative, and some comparisons:
> > * it's off by default. Turning it on by default will require making
> > django.forms.Form subclass SafeForm, which will almost certainly require
> > a huge and immediate upgrade effort, because SafeForm cannot have the
> > same API as Form. If it's off by default, I see no point at all in this
> > entire exercise. If we don't arrive at a point where the POST form
> > created in the tutorial is safe from CSRF, I think we've failed.
>
> My priority for this is a little different: while I would dearly love
> to see all Django applications secure against CSRF by default, I can't
> see a way of doing it that doesn't break existing apps. More important
> for me though is that the Django admin (and other reusable apps, such
> as Pinax stuff) MUST be secure against CSRF out of the box, no matter
> what the user puts in their settings.py file. If developers fail to
> protect themselves (especially when the solution is well documented)
> then at least it isn't our fault.
Given the massive (IMO) disadvantages to the SafeForm solution I've described
above, I think it's pretty disingenuous to say "well we provided you with the
tools, it's your fault if you're not protected". The solution needs to be
much better than well documented -- it needs to be easy to use, and easy to
change to from existing code. You also need to update all the existing Forms
documentation to tell people that really they shouldn't be using this. With
the amount of new stuff people need to learn to use SafeForm, they are just
not going to bother.
I guess the biggest thing here is we've got different priorities, as you said
-- I want to make it easy for developers to create apps that are secure
against CSRF, while you want to make the contrib apps secure, and want to
ensure no breakage even if people don't read release notes. The SafeForm
solution doesn't actually achieve that (see 6.3 and 6.4 above), and even if it
did, it is at the massive cost of baking in a CSRF solution which has such big
usability problems that I doubt anything outside contrib apps will adopt it.
In fact, if you simply want to fix contrib apps, SafeForm is a massively over-
engineered solution, as there are *much* less invasive ways to do it. All you
need to do is:
1) add the CSRF template tag to builtins and use it in templates.
2) add a @csrf_protect decorator on views.
3) hack RequestContext so that it always includes the CSRF context processor.
All contrib apps use RequestContext anyway, so the changes needed for that
method would be tiny, with no changes to settings.py required. This would
handle errors like the current middleware, but the error messages would only
occur when there is a genuine CSRF attack. (The reason I don't favour this
solution is because of criticisms 4 and 5 above).
The best test case for which solution is easier to use is to start by patching
all the contrib apps - forms, views, templates and tests. Then you have to
remember that you are expecting every developer out there to do the same with
their code base if the solution is really going to be a viable 'blessed'
solution.
I don't particularly want you to write that patch for SafeForm, because I
think it would be a waste of time, but I think that you'd be convinced of the
points above if you did. For comparison, the template tag required the
following changes to contrib apps:
View functions:
0 changes (because they all used RequestContext already, as will much
existing code - all that use generic views etc.)
Tests:
a single 1 line change to a test that counted the number of <input>s in an
admin page (which was a pretty fragile test that has been updated multiple
times already).
Templates:
18 templates - added {% load csrf %} and {% csrf_token %}
To give another example, here is the changeset for one real site (about 12,000
LOC) to switch to the template tag method:
http://bitbucket.org/spookylukey/cciw-website/changeset/f2edd690fd9e/
It has changes to settings and templates, but zero changes to view functions,
and zero changes to tests. That's better than some projects, because I
obviously must have already been using RequestContext for all POST forms, but
I don't think that will be atypical.
OK, I'm done!
Wondering-if-I-overdid-it-ly :-)
Luke
--
"Oh, look. I appear to be lying at the bottom of a very deep, dark
hole. That seems a familiar concept. What does it remind me of? Ah, I
remember. Life." (Marvin the paranoid android)
Luke Plant || http://lukeplant.me.uk/
Simon Willison
> All good points - the change in function signature naturally fell out
> of the CSRF work (since the form needs access to the request object in
> both cases) but you've convinced me that it's a step too far - I'll
> see if I can fix that.
back to the way it works in regular Django:
http://pypi.python.org/pypi/django-safeform/2.0.0
(Really should have used 0.1 / 0.2 etc for the version numbers, lesson
learnt for next time!)
I also added some unit testing sugar to address one of Luke's (many)
points, see the "Unit testing" section of the README file. I'm still
digesting the rest of his e-mail, plenty to think about there.
Cheers,
Simon
Simon Willison
> OK, here is my response. I hope this doesn't turn into a personal my-code-vs-
> your-code match (especially as most of "my code" is really other people's code
> and ideas :-) but I want to make sure we do this right, as it potentially has
> big implications, so the following will be "sleeves rolled up" (getting into
> details) and "gloves off" when it comes to criticisms of the code itself :-)
the best possible solution for this problem, so the more smart
discussion around it the better.
> Advantages of SafeForm
> ----------------------
>
> I can see the value of having contrib apps secured whatever the user has in
> their settings.py
>
> Unfortunately, that is the only advantage of any significance that I can
> see.
anywhere:
1. SafeForm has no dependency on Django's session framework. This is A
Good Thing as I personally avoid sessions in my projects (no need to
access state on every request if you can get away with not doing it).
2. SafeForm fails relatively gracefully in the event of CSRF check
failures - since it's tied in to the form validation flow, it displays
an inline error message but you don't lose your form data. This is my
personal favourite characteristic of SafeForm, partly because it
allows you to be much more strict with your CSRF tokens - you can
create tokens that expire relatively quickly without risking too much
harm to your users.
> Disadvantages of SafeForm
> -------------------------
>
> 1) Big changes are required to use it.
improvement there.
> * quite commonly, significant changes to the template:
> * Often you have to explicitly add the csrf token field. And you've
> got to worry about whether you need to or not.
Django silently blanks out {{ form.csrf_token }} (I've complained
about this before but here it's actually useful).
> * If you weren't displaying form.non_field_errors before (which can be
> common if you don't need it), you have to add it.
non_field_errors, you'll just lose the CSRF validation message - your
form will still preserve data and can be resubmitted. I noticed Flickr
fail silently like that if you fail a CSRF check (by tampering with
their magic_cookie) - they simply redisplay the form with your data
filled in, without any kind of error message.
> 2) More than one way to do CSRF protection.
>
> Although you provide a low level mechanism for forms, it still means you have
> Two Ways To Do CSRF Protection, and they are quite different (from the users
> point of view). Once you've added in the multiple forms in a <form> problem,
> you've now got Three Ways To Do CSRF Protection, all of which will be needed
> in quite common circumstances.
middleware method, which is a convenient way of adding CSRF protection
to an entire site, even if it's a bit ugly. Adding a new CSRF
protection mechanism is likely to significantly increase awareness of
the issue whatever we do, so I wouldn't be surprised to see a lot of
people start turning on the old middleware as a stop-gap measure. So
yes, this isn't exactly ideal.
> 3) More than one way to do Forms
> simpler; the API is possible to play with at the interactive prompt (see
> tests/regressiontests/forms.py, for example), unlike SafeForm which requires
> an HttpRequest object; and SafeForm sounds like something for the paranoid,
> ultra-secure types. So you can bet that tutorials and code across the
> internet are going to use Form.
documentation and community pressure. I'm perfectly willing to spend
the next year explaining CSRF to anyone who cares to hear about it -
it's shocking how poorly understood it is within the industry as a
whole.
> Because they have switched to using SafeForm, that form will now
> include the CSRF token, which is leaked to external sites, which is a security
> vulnerability.
be able to solve the CSRF problem without some serious end-user
education. If we're really worried about this we could set the default
token expiry time to half an hour to at least limit the potential
damage a bit.
(I know end-user education is rarely the answer to anything, but at
least we're dealing with developers here).
> 4) CSRF is baked in to view functions.
> the template tag method, this is easy to do. If someone has custom
> requirements, or because of their situation they can just do
> strict Referer checking or Origin checking or something, they can implement
> their own middleware and template tag, and switch *all* the Django apps
> over to their system simply by changing some settings.
really important issue it wouldn't be much work to add extensibility
hooks.
> * Testing view functions becomes a lot harder.
code: http://pypi.python.org/pypi/django-safeform/2.0.0#unit-testing
At the moment it requires using a different TestCase subclass, but I'd
be inclined to
suggest including that functionality in the default
django.test.TestCase should SafeForm ever make it in to trunk (I
personally think CSRF protection is too important a feature to
permanently live in the django.contrib namespace).
> With SafeForm, you have to remember to implement it for every POST form. If
> you forget one, you have a vulnerability, and you *won't know* until it is
> exploited. With lots of Django code being open source re-usable apps,
> targetted exploits that rely on knowing the source code are quite feasible.
omission pretty easy to spot in security audits. It's even possible to
envisage a crawler tool which looks out for forms that aren't
protected.
> 6) Backwards incompatibilities with contrib Forms.
with that. Breakage may be inescapable.
> I guess the biggest thing here is we've got different priorities, as you said
> -- I want to make it easy for developers to create apps that are secure
> against CSRF, while you want to make the contrib apps secure, and want to
> ensure no breakage even if people don't read release notes.
independently of sessions, middleware and RequestContext, but that's
mainly due to my own personal Django development preference.
So, to summarise... almost all of your points have merit (I think I've
resolved a couple of things relating to the weird form signature and
difficulty with unit testing in the 2.0.0 version). Some of them I
think are less serious than others. We have major philosophical
differences regarding developer education v.s. protect-by-default -
and I usually argue that education won't work, so I'm nearly on your
side on that one as well.
That said, the SafeForm approach deeply appeals to my personal style
of development - I like the inline error messages and the fact that it
works without middleware. I'm going to keep experimenting with it and
see if I can find ways of making it more palatable for general
consumption.
Cheers,
Simon
Luke Plant
> 1. SafeForm has no dependency on Django's session framework. This is A
> Good Thing as I personally avoid sessions in my projects (no need to
> access state on every request if you can get away with not doing it).
...
> It's a bit more than that - I also want something that works
> independently of sessions, middleware and RequestContext, but that's
> mainly due to my own personal Django development preference.
Just one thing to say in reply - the new version of CsrfViewMiddleware is also
independent of sessions. It sets its own cookie just like SafeForm does.
The references to the session framework still in there are purely for
backwards compatibility -- if a form is created and sent to the user with
Django 1.1, and the site is then upgraded to 1.2, when the user submits the
form the request will have a session cookie and a CSRF token, but no CSRF
cookie, which would produce a 403 error. So for that tiny little window, we
allow a fall-back to using the 1.1 method by allowing a session cookie for a
CSRF cookie. The fall-back doesn't actually trigger any session activity - it
just looks at request.COOKIES[settings.SESSION_COOKIE_NAME].
I also had one thing to add to my previous e-mail - on the backwards
compatibility front, there are also the tests that people may have written
against customised contrib views. I have examples of these in my projects --
where I have added significant customisations to the admin, and I want to test
certain post-conditions etc. In these cases, we also have potential for
breakage - the tests are going to fail until they are updated in the same way
that core tests need to be updated to get and use the CSRF token. This will
happen even if people have used 'best practice' (using super() and inheritance
so they don't duplicate any actual contrib code or templates). The examples I
have in my code actually would not break, because I happen to have used twill
for those tests, but I think the point stands. (You could argue that 'best
practice' for tests means using twill-like tests, but Django tests have not
provided either a good example or helpful utilities on this front, so it's an
unreasonable expectation IMO).
Broken tests are not as bad as breaking the actual site, but any responsible
developer is going to fix rather than ignore failing tests, so it definitely
adds to the upgrade cost.
Regards, and thanks for taking my (lengthy) criticisms in good spirit!
Luke
--
OSBORN'S LAW
Variables won't, constants aren't.
Luke Plant || http://lukeplant.me.uk/
Russell Keith-Magee
>
> Hi Simon,
>
> OK, here is my response. I hope this doesn't turn into a personal my-code-vs-
> your-code match (especially as most of "my code" is really other people's code
> and ideas :-) but I want to make sure we do this right, as it potentially has
> big implications, so the following will be "sleeves rolled up" (getting into
> details) and "gloves off" when it comes to criticisms of the code itself :-)
Completely understood, and right back at you :-)
> Advantages of SafeForm
> ----------------------
>
> I can see the value of having contrib apps secured whatever the user has in
> their settings.py
>
> Unfortunately, that is the only advantage of any significance that I can
> see. And there are *much* easier ways to achieve that goal if that is our
> main priority (see the bottom of my e-mail for details).
>
> There is the 'advantage' that Russell mentioned of not having to touch the
> tutorials. This is a pretty dubious advantage IMO because it means that the
> tutorials will still lead to insecure code. Far from 'raising the profile' of
> CSRF as Simon rightly suggested we should do, this is just keeping quiet about
> it.
The "advantage" here is a pedagogical one. When we're writing a basic
introductory materials, you want to avoid introducing unnecessary
details. Every time you say "don't try to understand this bit, just do
it", you introduce a conceptual magic wall that you have to break down
later. IMHO, there is some advantage to being able to defer the
discussion of building CSRF safe forms until you are explicitly
discussing security issues.
That said, it's a fairly minor advantage as long as the changes
required on the end user are also minor. If all we need to say is "put
this template tag in your form for security reasons - we will tell you
why later" then I'm not so concerned with this particular problem.
However, as the size and scope of the ignored parts become more
involved, my pedagogical objections grow.
> Disadvantages of SafeForm
> -------------------------
>
> 1) Big changes are required to use it.
>
> The changes to use SafeForm are much more intrusive than using the template
> tag. It requires quite extensive changes to both templates and views:
>
> * different API and different flow control for SafeForm compared to Form (it
> may be better, but it's different, which means a big upgrade cost)
> * a decorator for views
> * a decorator for forms
> * quite commonly, significant changes to the template:
> * Often you have to explicitly add the csrf token field. And you've
> got to worry about whether you need to or not.
> * If you weren't displaying form.non_field_errors before (which can be
> common if you don't need it), you have to add it.
> * For the multiple forms in a <form> case, you've got even more changes
> to your view function and your template, and you have to learn about
> CsrfForm on top of everything else.
>
> Using SafeForm is a *lot* more complex than the template tag method. The
> basic instructions for day-to-day usage of the template tag method (i.e. not
> including change-once settings) go like this:
I will agree that SafeForm is intrusive - but IMHO it's intrusive in a
familiar way. There are a lot of changes required, but these changes
are just more decorators, forms, etc - the things that form the bread
and butter of Django development.
The template tag also requires intrusive changes - just in areas that
are less "familiar". To fully grok how the template tag works, you
need to grok the interplay of middleware, context processors, and
template tag loading - and while these are all core parts of Django,
the interplay of these components isn't part of the everyday Django
development experience.
> If the target of a <form> is internal:
> * add {% load csrf %} to the template and {% csrf_token %} to the form
> * use RequestContext in the corresponding view.
>
> And that's it.
... once you've also got your INSTALLED_APPS, CONTEXT_PROCESSORS and
MIDDLEWARE_CLASSES set up correctly - and make sure you have, because
if you haven't you're going to get weird errors - or worse still,
accidentally disable CSRF protection.
To be clear - my concern isn't with the template tag itself. I think
that part is quite elegant. My concerns stem almost entirely from the
dance that is required to make the whole thing work right in the first
place.
That said, I think your remaining criticisms about SafeForm are
entirely valid, and I can't really offer much by way of defense. The
SafeForm approach as implemented is about as good as it will ever get
- we might be able to tinker around the edges a bit to fix some of the
issues you describe with testing, but fundamentally, SafeForm requires
that end-users use a different type of Form, and that will require
lots of code churn and the related complications you raised.
So - is there anything we do to make the template tag more palatable?
I think we can - and you've already shown how:
> In fact, if you simply want to fix contrib apps, SafeForm is a massively over-
> engineered solution, as there are *much* less invasive ways to do it. All you
> need to do is:
> 1) add the CSRF template tag to builtins and use it in templates.
> 2) add a @csrf_protect decorator on views.
> 3) hack RequestContext so that it always includes the CSRF context processor.
>
> All contrib apps use RequestContext anyway, so the changes needed for that
> method would be tiny, with no changes to settings.py required.
The unofficial definition of django.contrib is "optional, de facto
standard implementations of common patterns" - but I don't think
that's a reasonable description of CSRF at this point in the life of
the web. CSRF protection isn't (or shouldn't be) optional - it's part
of best practice. I can see no reason why we shouldn't be baking CSRF
protection into the Django core - and if we do that, a lot of my
objections to the template tag approach go away.
Using your list of suggestions - (1) removes the need for the
INSTALLED_APPS modification and the {% load csrf %} template
modification, and (3) removes the need for the CONTEXT_PROCESSORS
modification.
That leaves (2) and the middleware.
> This would
> handle errors like the current middleware, but the error messages would only
> occur when there is a genuine CSRF attack. (The reason I don't favour this
> solution is because of criticisms 4 and 5 above).
A middleware is really the only way to ensure that every view is
protected by default - which is a certainly a desirable goal. However,
if adding CSRF protection requires _any_ changes on the part of the
end user, then I can guarantee that somewhere, a
lazy/time-malnourished developer will take the "just disable the
middleware and make it work" approach. As soon as this happens,
contrib.admin becomes insecure. On this point, I completely agree with
Simon: End users should be allowed to be as lazy as they like, but
their laziness shouldn't open security holes in an app that Django
ships, since the contrib apps (and admin in particular) are the
obvious first port of call for any systematic CSRF attack.
So - let's have both. A middleware enabled by default protects the
rest of the views. However, we can also have a view decorator lets us
protect admin (and other contrib apps) explicitly. If users disable
the middleware, admin remains secure. If users fail to follow the
migration instructions correctly, admin remains secure. Users like
Simon that were tormented by middlewares when they were younger ( :-)
) can disable the middleware and use the decorator liberally
throughout their own projects; the worst possible outcome here is that
one of their own views is insecure - admin is unaffected.
The middleware and view can easily be made to play nicely together -
there's no need to check the token twice, so make the middleware set a
flag on the response object that marks the content as CSRF-checked; if
the decorator sees that flag, it doesn't need to duplicate the check.
At this point, I have a lot less objection to the template-tag
solution. The potential for configuration or upgrade failures is
significantly reduced, and the code churn required is minimal.
In an ideal world, I still have some wishlist items, but I'm not sure
the extent to which they are possible:
* The SafeForm method of reporting errors as part of form validation
is much nicer than the 403 page, IMHO. However, I'm not sure what we
can do to provide form-level CSRF error handling without introducing a
whole bunch of other fragility.
* The requirement for using RequestContext is somewhat annoying. I
can't see any obvious way to piggyback CSRF data onto the base Context
itself, but perhaps we can make RequestContext a more prominent
default? The most obvious solution here is to make render_to_response
use RequestContext by default. This was proposed a _long_ time ago
(#650 - back when RequestContext was called DjangoContext) and
rejected. However, if we're going to increase the significance of
RequestContext, maybe we should revisit this.
Yours
Russ Magee %-)
Luke Plant
> On Fri, Sep 18, 2009 at 6:09 AM, Luke Plant <L.Pla...@cantab.net> wrote:
> > If the target of a <form> is internal:
> > * add {% load csrf %} to the template and {% csrf_token %} to the form
> > * use RequestContext in the corresponding view.
> >
> > And that's it.
>
> ... once you've also got your INSTALLED_APPS, CONTEXT_PROCESSORS and
> MIDDLEWARE_CLASSES set up correctly - and make sure you have, because
> if you haven't you're going to get weird errors - or worse still,
> accidentally disable CSRF protection.
Fair point - I was explicitly talking about the *day-to-day* usage at that
point. For the other things, the errors aren't quite so inscrutable any more
- the 403 page looks like this now:
http://lukeplant.me.uk/uploads/django_csrf_403.html
The only way to *accidentally* disable the protection and not know is if you
remove the middleware - other changes will result in an error. In reality,
this is going to happen when people comment it out for the sake of debugging,
and forget to put it back in again, or just decide they don't care. I can see
that will happen more than you'd hope...
...but is it really our fault if people disable the protection? I guess to the
extent that we've made it difficult to get right, or more tempting to do it
wrong, it *is* our fault.
So I don't think we necessarily need to make it *impossible*, just minimise
the incentive.
> So - is there anything we do to make the template tag more palatable?
>
> I think we can - and you've already shown how:
> > In fact, if you simply want to fix contrib apps, SafeForm is a massively
> > over- engineered solution, as there are *much* less invasive ways to do
> > it. All you need to do is:
> > 1) add the CSRF template tag to builtins and use it in templates.
> > 2) add a @csrf_protect decorator on views.
> > 3) hack RequestContext so that it always includes the CSRF context
> > processor.
> >
> > All contrib apps use RequestContext anyway, so the changes needed for
> > that method would be tiny, with no changes to settings.py required.
OK, I agree that this solution has some nice features, but let's be realistic
about its failings:
1) changing RequestContext so it always includes the CSRF context processor is
... 'cowboy', as I think Simon would put it :-) I wasn't actually suggesting
that as a real proposal.
2) We would still have to fix all the tests, because we can't just turn the
decorator off. There are also the tests that we can't fix, i.e. where other
people have customised contrib apps in various ways and written tests against
the views. I mentioned this in my most recent reply to Simon. I'm not so
bothered about having to fix ours, it's other people's that worry me, and
the DRY violation - baking it in like this means that every test of a POST
view is now concerned with CSRF.
I can think of some hacks to avoid fixing tests (ours and other peoples):
* make the decorator turn itself off:
* on the basis of some setting. You can be sure that this will suffer from
the same problem as the middleware setting - people will turn it off for
production.
* on the basis of some other way of detecting test suite mode, whether
'runtests.py' or 'django-admin.py test'
* hacking test.Client.post() to always include a CSRF cookie and token.
Easy to do, but fairly 'cowboy'.
I'm guessing it could also break things that depend on the
presence/absence/number of cookies.
I think the lazy instinct which makes me not want to fix even our own tests is
a healthy instinct at root...but I don't know how to address it best.
> A middleware is really the only way to ensure that every view is
> protected by default - which is a certainly a desirable goal. However,
> if adding CSRF protection requires _any_ changes on the part of the
> end user, then I can guarantee that somewhere, a
> lazy/time-malnourished developer will take the "just disable the
> middleware and make it work" approach. As soon as this happens,
> contrib.admin becomes insecure. On this point, I completely agree with
> Simon: End users should be allowed to be as lazy as they like, but
> their laziness shouldn't open security holes in an app that Django
> ships, since the contrib apps (and admin in particular) are the
> obvious first port of call for any systematic CSRF attack.
Perhaps we could add some code to the template tag that checks
MIDDLEWARE_CLASSES for the CSRF middleware and if it's not there, and it's on
a production site, posts some information to a public web site to name and
shame the developer, and e-mails their boss as well... :-)
Seriously, another option is to make AdminSite.check_dependencies require the
middleware. It already has checks for INSTALLED_APPS and
TEMPLATE_CONTEXT_PROCESSORS - i.e. the admin will just refuse to work if you
don't have these things set up correctly to include the CSRF stuff. I
deliberately avoided adding the CSRF middleware to that list because I wanted
it to be possible to just disable the middleware for debugging/testing
purposes. I'm not sure that's so necessary now - if we are wanting to say "No
You Can't Turn Off CSRF Protection For The Admin You Stupid Lazy Developer!",
then AdminSite.check_dependencies() is actually a pretty good place to do it.
Also, we would need to turn AdminSite.check_dependencies() on even if DEBUG is
off, and we need to call in it get_urls(). For some reason, it's currently
only called in AdminSite.root(), which is now deprecated. This is surely an
oversight, right? Any reason why it's not in get_urls() ? I'm guessing there
is a good reason it's not called from __init__().
The remaining problem would be how to make AdminSite.check_dependencies() not
require the middleware if the test suite is being run. (Alternatively, fix the
tests, or one of the other testing hacks described above). I've hacked
together a solution:
http://bitbucket.org/spookylukey/django-trunk-lukeplant/changeset/9366c823903d/
Sadly, a kitten had to be killed in the process.
That, of course, will only cover the admin for definite, but it is probably
the most used contrib app, and the most obvious thing to try to exploit, as
you say.
With these changes, I personally don't see the need to bake the CSRF template
tag library and context processor into the core, or to use a decorator. If
someone disables the middleware, they break the admin. That's a fairly big
disincentive, so just fixing their app is easier.
Also if we leave it in contrib namespace, it does allow the possibility of
people swapping out the implementation very easily, just by changing settings,
which I think is a distinct advantage.
> * The requirement for using RequestContext is somewhat annoying. I
> can't see any obvious way to piggyback CSRF data onto the base Context
> itself, but perhaps we can make RequestContext a more prominent
> default? The most obvious solution here is to make render_to_response
> use RequestContext by default. This was proposed a _long_ time ago
> (#650 - back when RequestContext was called DjangoContext) and
> rejected. However, if we're going to increase the significance of
> RequestContext, maybe we should revisit this.
Yeah, I'd like a builtin shortcut like that - used like this:
render(request, 'template_name.html', {'foo':bar })
The biggest problem, for me, is finding a decent name - since
'render_to_response' is already taken. Maybe something as vague as
'standard_response' would work.
Regards,
Luke Plant
> So - let's have both. A middleware enabled by default protects the
> rest of the views. However, we can also have a view decorator lets us
> protect admin (and other contrib apps) explicitly. If users disable
> the middleware, admin remains secure. If users fail to follow the
> migration instructions correctly, admin remains secure. Users like
> Simon that were tormented by middlewares when they were younger ( :-)
> ) can disable the middleware and use the decorator liberally
> throughout their own projects; the worst possible outcome here is that
> one of their own views is insecure - admin is unaffected.
>
> The middleware and view can easily be made to play nicely together -
> there's no need to check the token twice, so make the middleware set a
> flag on the response object that marks the content as CSRF-checked; if
> the decorator sees that flag, it doesn't need to duplicate the check.
Another advantage of the middleware + decorator solution is that sometimes
there might be a legitimate case for turning off the middleware globally e.g.:
- if for some reason there are no POST forms, and never will be.
- if all POST interaction is done using AJAX
- if the user has implemented their own solution for CSRF for
their own apps.
In this situation, it's nice if the developer can turn off the middleware and
still have the admin protected.
In trying to implement this, I came across the problem that the same decorator
can't be used for methods and functions, and I need to decorate some methods
of AdminSite. (AdminSite uses never_cache directly on methods quite a bit, but
this only works by accident - never_cache doesn't use the view function
arguments (e.g. the request object), otherwise it would fail). Does anyone
have a nice solution to this?
Regards,
Luke
--
"Outside of a dog, a book is a man's best friend... inside of a dog,
it's too dark to read."
Luke Plant || http://lukeplant.me.uk/
Russell Keith-Magee
>
> On Saturday 19 September 2009 16:56:52 Russell Keith-Magee wrote:
>
>> On Fri, Sep 18, 2009 at 6:09 AM, Luke Plant <L.Pla...@cantab.net> wrote:
>> > If the target of a <form> is internal:
>> > * add {% load csrf %} to the template and {% csrf_token %} to the form
>> > * use RequestContext in the corresponding view.
>> >
>> > And that's it.
>>
>> ... once you've also got your INSTALLED_APPS, CONTEXT_PROCESSORS and
>> MIDDLEWARE_CLASSES set up correctly - and make sure you have, because
>> if you haven't you're going to get weird errors - or worse still,
>> accidentally disable CSRF protection.
>
> Fair point - I was explicitly talking about the *day-to-day* usage at that
> point. For the other things, the errors aren't quite so inscrutable any more
> - the 403 page looks like this now:
>
> http://lukeplant.me.uk/uploads/django_csrf_403.html
I agree that this makes the reasons a lot clearer, but I stand by my
criticism that it's still a metric buttload of instructions just to
make a basic form submit.
My concern here is that every time you add an item to a list of
migration instructions, you increase the possibility that something
can go wrong. In turn, this increases the likelihood that a potential
convert will get frustrated and declare that "Django sucks" - or
worse, that an existing convert will declare "WTF" and go somewhere
else. In this case, the thing that is going to go wrong is "submitting
forms", which is pretty much the bread and butter of a web framework.
IMHO, anything we can do to limit the number of things that can go
wrong, the better.
> ...but is it really our fault if people disable the protection? I guess to the
> extent that we've made it difficult to get right, or more tempting to do it
> wrong, it *is* our fault.
I'm not sure that this is a distinction that users are going to make
when their site gets hacked. I would bet money that regardless of the
cause, Django will wear the blame. If the problem was caused by the
end user disabling CSRF protection against Django's advice, then it
will be Django's fault that it was possible to disable it at all.
>> So - is there anything we do to make the template tag more palatable?
>>
>> I think we can - and you've already shown how:
>> > In fact, if you simply want to fix contrib apps, SafeForm is a massively
>> > over- engineered solution, as there are *much* less invasive ways to do
>> > it. All you need to do is:
>> > 1) add the CSRF template tag to builtins and use it in templates.
>> > 2) add a @csrf_protect decorator on views.
>> > 3) hack RequestContext so that it always includes the CSRF context
>> > processor.
...
>> >
>> > All contrib apps use RequestContext anyway, so the changes needed for
>> > that method would be tiny, with no changes to settings.py required.
>
> OK, I agree that this solution has some nice features, but let's be realistic
> about its failings:
>
> 1) changing RequestContext so it always includes the CSRF context processor is
> ... 'cowboy', as I think Simon would put it :-) I wasn't actually suggesting
> that as a real proposal.
I was :-) Or rather, I'm not thinking about this as an 'always
included context processor', but as a core piece of functionality that
we should bake into RequestContext. CSRF is a very big problem at the
core of rendering forms. Why not make CSRF protection a core part of
the RequestContext contract?
> 2) We would still have to fix all the tests, because we can't just turn the
> decorator off. There are also the tests that we can't fix, i.e. where other
> people have customised contrib apps in various ways and written tests against
> the views. I mentioned this in my most recent reply to Simon. I'm not so
> bothered about having to fix ours, it's other people's that worry me, and
> the DRY violation - baking it in like this means that every test of a POST
> view is now concerned with CSRF.
>
> I can think of some hacks to avoid fixing tests (ours and other peoples):
Agreed that we shouldn't require wholesale updates to test suites.
The test suite already does some setup/teardown to make a testable
environment - for example, swapping out the SMTP outbox - removing or
mocking CSRF protection could easily fit into this box.
> I think the lazy instinct which makes me not want to fix even our own tests is
> a healthy instinct at root...but I don't know how to address it best.
Agreed, on both counts.
> Seriously, another option is to make AdminSite.check_dependencies require the
> middleware. It already has checks for INSTALLED_APPS and
> TEMPLATE_CONTEXT_PROCESSORS - i.e. the admin will just refuse to work if you
> don't have these things set up correctly to include the CSRF stuff. I
> deliberately avoided adding the CSRF middleware to that list because I wanted
> it to be possible to just disable the middleware for debugging/testing
> purposes. I'm not sure that's so necessary now - if we are wanting to say "No
> You Can't Turn Off CSRF Protection For The Admin You Stupid Lazy Developer!",
> then AdminSite.check_dependencies() is actually a pretty good place to do it.
My issue with this approach is that while admin is extremely common,
it's not ubiquitous. There are Django sites that don't use admin;
binding the safety check to a component that isn't mandatory doesn't
strike me as a good idea.
> Also, we would need to turn AdminSite.check_dependencies() on even if DEBUG is
> off, and we need to call in it get_urls(). For some reason, it's currently
> only called in AdminSite.root(), which is now deprecated. This is surely an
> oversight, right? Any reason why it's not in get_urls() ? I'm guessing there
> is a good reason it's not called from __init__().
I'll need to check to be sure, but I suspect this is oversight.
> With these changes, I personally don't see the need to bake the CSRF template
> tag library and context processor into the core, or to use a decorator. If
> someone disables the middleware, they break the admin. That's a fairly big
> disincentive, so just fixing their app is easier.
As I said earlier - the less things that can go wrong, the better.
Putting the tag and context processor removes 3 of the 5 items on the
'things that might be wrong' list on the 403 page. That's three things
the end user can't possibly get wrong.
> Also if we leave it in contrib namespace, it does allow the possibility of
> people swapping out the implementation very easily, just by changing settings,
> which I think is a distinct advantage.
I'm not sure this is a huge selling point for me. CSRF protection
isn't one of those things where the community benefits from having
multiple options. I think there is much more to be gained by having a
solid, well defined framework at the core of Django that everyone
knows, understands and relies upon.
Plus, if you _really_ want a different CSRF protection framework, you
can still use a contrib library - you just can't use {% csrf token %}
as a tag name.
Yours,
Russ Magee %-)
Luke Plant
the migration issues and the fact that it is security related, I guess I can
stomach it.
I've updated the patch [1] to move things to builtin functionality. I also
had to fix some bugs to get the csrf_protect decorator working for methods,
which are in trunk already.
I've left most of the code itself under django/contrib/csrf because:
1) backwards compatibility with people importing the middleware
means we have to leave django/contrib/csrf for some things
anyway.
2) In this case, I don't see any great advantage in having stub modules
which just import other stuff for backwards compatibility
3) I really can't be bothered to change all the imports
at this point in time!
I moved the template tag itself to core code, because it was causing import
cycles otherwise, and there are no backwards compatibility issues, nor does it
add any actual imports of contrib code to core.
I think the patch now addresses all your/our concerns:
- decorator on all contrib view functions that need it.
I'm pretty sure I got them all, but I only decorated the ones
that actually need it (i.e. use POST), and I only added it
on the 'source' function (e.g. auth.views.password_change and not
AdminSite.password_change) because otherwise
you end up decorating many times. I also added the decorator
in AdminSite.admin_view, which will be a catch-all for all
admin views (provided users are using AdminSite.urls)
- the view middleware is still on by default.
- csrf_token is in builtin templatetags
- csrf context_processor is hard coded into RequestContext
- tests 'fixed' with very little work.
I fixed the tests by a custom attribute on request objects that tells the
middleware/decorator to not actually reject requests. This is better than
disabling completely, because it means that the middleware will still send
cookies etc., and it's always good to have tests as close as possible to the
real code. The test client adds the custom attribute to HttpRequests after it
has created them. I had to add the attribute in one other place in the code
as well - a test that was manually calling a view function that had
csrf_protect applied.
This method of fixing tests was also the best for testing the CSRF middleware
- globally mocking the middleware out would have made it hard to test the
middleware itself!
Docs are all updated, all tests passing etc.
If people are happy for this to go in, it would be very helpful if other
people could have a go updating their apps and give the general docs/upgrade
instructions/tutorials a good check after I commit it. I can't easily do
checks like that, because I just won't spot the holes after having the code in
my head for so long.
The only thing left is a nicer render_to_response shortcut for using
RequestContext, which is a refinement we can add later.
Regards,
Luke
[1]
http://code.djangoproject.com/attachment/ticket/9977/csrf_template_tag_r11587_1.diff
--
Parenthetical remarks (however relevant) are unnecessary
Luke Plant || http://lukeplant.me.uk/
Simon Willison
wrote:
> their laziness shouldn't open security holes in an app that Django
> ships, since the contrib apps (and admin in particular) are the
> obvious first port of call for any systematic CSRF attack.
>
> So - let's have both. A middleware enabled by default protects the
> rest of the views. However, we can also have a view decorator lets us
> protect admin (and other contrib apps) explicitly.
> * The SafeForm method of reporting errors as part of form validation
> is much nicer than the 403 page, IMHO. However, I'm not sure what we
> can do to provide form-level CSRF error handling without introducing a
> whole bunch of other fragility.
(in particular) not using a form submission are the single thing that
makes me favour SafeForm over the template tag approach for my own
stuff. That's why I designed SafeForm to complement the forms
framework - correctly handling CSRF feels like a form validation
problem to me, and reusing Django's redisplay-the-form-with-errors
flow is a really neat thing to be able to do.
The main reason I really like preserving form data is that it means
CSRF failures are less of a problem, allowing us to be much more
strict about how they work (setting form tokens to expire after a few
hours, tying tokens to individual forms etc). This means we can reduce
the damage caused in the case of a token leaking somehow. This becomes
particularly important if tokens are going to be used to protect GET
requests, which some applications may want to do (Flickr have CSRF-
protected GETs for their logout and change-language links, for
example) - GET tokens are more likely to leak in other people's
referrer logs or to intermediate proxies.
There is a way we could achieve form validation, but it isn't
particularly pretty. Say the CSRF check is performed by a decorator
(which has an accompanying middleware for applying that decorator to
the entire site). The problem we have is that in the case of a
failure, the fact that the check failed needs to be communicated
through to the form /inside/ that view. The form doesn't have access
to the request, so we're stuck.
But... the form does have access to request.POST. The dirty solution
would be for the decorator to rewrite request.POST to include a new
key:
request.POST.mutable = True
request.POST['__validation_must_fail'] = True
request.POST.mutable = False
django.forms.Form.is_valid() could then be hard-coded to always fail
if '__validation_must_fail' is present in the data dictionary. I've
avoided including the string "csrf" in the form key because this
feature feels like it could be useful for other things as well (I
might use it for testing out my form error styles). I don't think it's
a security vulnerability - if someone fakes a "__validation_must_fail"
key in their own submission I don't see that there's anything bad they
can do.
It's a pretty monstrous hack, but it would allow a CSRF protecting
decorator to inform a form inside a view that the check had failed and
the form should not validate.
I wouldn't suggest this as the default behaviour for the overall CSRF
protection scheme - it would leave hand-rolled forms unprotected, and
I imagine there are new error cases it would introduce. That said, it
would placate users like myself who want the nicer invalidation
messages and are willing to do a bit of work to achieve them. Maybe
something like this:
@csrf_protect
def view(request):
"""A view that fails the CSRF check with a full page 403,
as seen in Luke's current middleware"""
@csrf_protect_form
def view(request):
"""A view that won't 403 directly, but will ensure that any
calls to form.is_valid() fail due to the presence of a
__validation_must_fail key"""
Thinking more pie-in-the-sky-ly, maybe this approach could be
generalised further, to specify a mechanism by which request.POST can
include a set of validation errors that should be shown by the form.
This might have other uses in things like wizards where validation
errors may want to persist between different pages.
> * The requirement for using RequestContext is somewhat annoying. I
> can't see any obvious way to piggyback CSRF data onto the base Context
> itself, but perhaps we can make RequestContext a more prominent
> default? The most obvious solution here is to make render_to_response
> use RequestContext by default.
how horrible the syntax for using it is:
render_to_response('template.html', {
'name': 'Foo',
'blah': 'baoenuth',
}, context_instance=RequestContext(request))
<shudder>
Yeah, I'd like a builtin shortcut like that - used like this:
render(request, 'template_name.html', {'foo':bar })
The biggest problem, for me, is finding a decent name - since
'render_to_response' is already taken. Maybe something as vague as
'standard_response' would work.
> Yeah, I'd like a builtin shortcut like that - used like this:
> render(request, 'template_name.html', {'foo':bar })
>
> The biggest problem, for me, is finding a decent name - since
> 'render_to_response' is already taken. Maybe something as vague as
> 'standard_response' would work.
template, dictionary)" function somewhere as a result.
I'd be enormously happy to see that go in as django.shortcuts.render -
the name may not be as descriptive as render_to_response but for
something used in practically every view I think terseness should beat
descriptiveness. Alternatively, I've been experimenting recently with
a TemplateResponse class which lets you do this:
def view(request):
return TemplateResponse(request, 'my_template.html', {'foo':
'bar'})
It's an HttpResponse class that's designed to be lazily evaluated.
This makes it really useful for subclassing - you can do things like
this:
class MyGenericView(object):
def view(self, request):
# Do something complicated
return TemplateResponse(request, 'my_template.html', {'foo':
'bar'})
class MyCustomisedView(object):
def view(self, request):
response = super(MyCustomisedView, self).view(request)
response.context['foo'] = 'baz'
if is_mobile_phone(request):
response.template = 'my_mobile_template.html'
return response
This pattern would be particularly valuable for customising the admin,
which currently uses nasty extra_context argument hacks to achieve
something that isn't nearly as useful.
My current implementation of TemplateResponse is here:
http://github.com/simonw/django-openid/blob/master/django_openid/response.py
- there's a SimpleTemplateResponse as well which doesn't use
RequestContext (but has a more verbose name to discourage its use).
That was a bit of a tangent... but to conclude:
1. I've grudgingly accepted that a template tag based solution is
much, much more practical for inclusion in Django than the SafeForm
option, purely due to the difficulty in porting existing code and the
differences between the two form APIs.
2. I'm not at all keen on the implementation as a middleware
(especially a view middleware, which doesn't play well with generic
views and redispatching to other view functions, both patterns I like
a lot). I'd be perfectly happy with a decorator that is also available
as a middleware.
3. I'd like to include support for CSRF form tokens to expire and be
locked down to individual forms, as seen in SafeForm. Whether or not
we actually go for that depends on how likely we are to find a
solution to the next point...
4. I'd love it if we could find a way for developers who care (such as
myself) to opt-in to CSRF-failing gracefully at the form validation
level. I'm confident this should be possible, but I don't think it's
necessary to make it the default behaviour (the CSRF 403 screen is
probably fine for most people).
Luke: any thoughts on the above? I'd be happy to contribute code.
Cheers,
Simon
Russell Keith-Magee
>
> OK, you convinced me. I really would rather this wasn't baked in, but given
> the migration issues and the fact that it is security related, I guess I can
> stomach it.
>
> I've updated the patch [1] to move things to builtin functionality. I also
> had to fix some bugs to get the csrf_protect decorator working for methods,
> which are in trunk already.
>
> I've left most of the code itself under django/contrib/csrf because:
>
> 1) backwards compatibility with people importing the middleware
> means we have to leave django/contrib/csrf for some things
> anyway.
> 2) In this case, I don't see any great advantage in having stub modules
> which just import other stuff for backwards compatibility
This isn't just a "moving deckchairs on the Titantic" thing - I can
think of at least three good reasons we should be moving the code into
core modules:
* Maintaining the basic contract of django.contrib - that you should
be able to delete the contrib directory and have Django still work. If
all the CSRF code is in contrib, this won't be the case.
* Making it clear that this really is a core feature - it's a core
template tag, a core middleware, and a core context. Just like the URL
is part of the UI for a web app, the module namespace is part of the
UI for a library like Django.
* For deprecation purposes, we can say that the whole contrib.csrf
module will be deprecated in 1.4 (at which point, CSRF will be an
entirely core feature).
> 3) I really can't be bothered to change all the imports
> at this point in time!
I can understand this as interim measure while we get consensus with
other core developers, but I'd be -1 to leaving the code as is for the
final commit.
> I moved the template tag itself to core code, because it was causing import
> cycles otherwise, and there are no backwards compatibility issues, nor does it
> add any actual imports of contrib code to core.
Which reinforces the point. Some of the code is going to be in core.
Do you want to write the documentation explaining why some code is in
contrib and some isn't?
> I think the patch now addresses all your/our concerns:
Agreed.
> I fixed the tests by a custom attribute on request objects that tells the
> middleware/decorator to not actually reject requests. This is better than
> disabling completely, because it means that the middleware will still send
> cookies etc., and it's always good to have tests as close as possible to the
> real code. The test client adds the custom attribute to HttpRequests after it
> has created them. I had to add the attribute in one other place in the code
> as well - a test that was manually calling a view function that had
> csrf_protect applied.
>
> This method of fixing tests was also the best for testing the CSRF middleware
> - globally mocking the middleware out would have made it hard to test the
> middleware itself!
I'm reasonably happy with the testing approach you've taken - or, at
least, I can't think of anything substantially better.
My only comment is that we might want to put an underscore on the
magic variable (i.e., request._dont_enforce_csrf_checks) to reinforce
the fact that this really isn't public API.
> Docs are all updated, all tests passing etc.
>
> If people are happy for this to go in, it would be very helpful if other
> people could have a go updating their apps and give the general docs/upgrade
> instructions/tutorials a good check after I commit it. I can't easily do
> checks like that, because I just won't spot the holes after having the code in
> my head for so long.
Whew. Well, that was a year well spent :-)
At this point, I'm convinced, mod the minor things I've flagged.
However, I'd like to see Jacob and Malcolm chime in before this is
committed.
> The only thing left is a nicer render_to_response shortcut for using
> RequestContext, which is a refinement we can add later.
Agreed, and it's a mostly orthogonal change anyway.
Russ %-)
James Bennett
> Yeah, I'd like a builtin shortcut like that - used like this:
> render(request, 'template_name.html', {'foo':bar })
> The biggest problem, for me, is finding a decent name - since
> 'render_to_response' is already taken. Maybe something as vague as
> 'standard_response' would work.
FWIW most people I know seem to be using the direct_to_template
generic view for this; its argument signature is close to what's
desired, and it uses RequestContext.
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
Jacob Kaplan-Moss
<freakb...@gmail.com> wrote:
> At this point, I'm convinced, mod the minor things I've flagged.
> However, I'd like to see Jacob and Malcolm chime in before this is
> committed.
I've mostly stayed out of the discussion because I haven't had much
helpful to say that isn't being better expressed by someone else. But
for the record I am following this closely, and it seems to me that
y'all are narrowing in on a pretty good solution.
That is, making CSRF protection built-in seems to be the best
approach. I did a quick survey of other web frameworks' CSRF
protection, and found:
* CSRF protection is an optional component (something like SafeForm)
in Pylons and TurboGears, and nobody seems to use it (judging by the
lack of documentation, lack of examples, and lack of questions about
it on mailing lists)
* CSRF protection is optional (again, something like SafeForm) in
Symfony and CakePHP, and nobody seems to use it (similar criteria).
* CSRF protection is a built-in-but-optional bit in Zend (you can add
a "csrf field" any form to get automatic CSRF protection), and it
seems to be used regularly.
* Rack::CSRF provides middleware-level CSRF protection to Rack apps,
and seems to be used with microframeworks (e.g. Sinatra) regularly.
* Ruby on Rails provides built-in, completely transparent CSRF
protection, and nearly everyone uses it.
Based on this quick-and-dirty evaluation, it seems the unifying factor
is that nobody really uses CSRF protection unless (a) it's built in or
(b) it's too late.
Or, put another way, how many people got template auto-escaping right
before we made it automatic?
I'm gonna give Luke's latest a try tonight if I can, but it looks pretty good.
Jacob
PS: I'm with Simon that we need a better shortcut for
render-with-request-context. I'm gonna have to think a bit more about
what that should be, though.
Luke Plant
> On Tue, Sep 22, 2009 at 10:34 AM, Luke Plant <L.Pla...@cantab.net> wrote:
> > I've left most of the code itself under django/contrib/csrf because:
> >
> > 1) backwards compatibility with people importing the middleware
> > means we have to leave django/contrib/csrf for some things
> > anyway.
> > 2) In this case, I don't see any great advantage in having stub modules
> > which just import other stuff for backwards compatibility
>
> This isn't just a "moving deckchairs on the Titantic" thing - I can
> think of at least three good reasons we should be moving the code into
> core modules:
>
> * Maintaining the basic contract of django.contrib - that you should
> be able to delete the contrib directory and have Django still work. If
> all the CSRF code is in contrib, this won't be the case.
That doesn't happen to be true at the moment - django.core.context_processors
depends on django.contrib.auth. (Not saying that's a good thing, just
highlighting a fact). But on the other hand, you can at least disable the auth
context processor, and you can't disable the CSRF one.
The other reason for leaving it as it is at the moment is for the sake of the
patch - it's much harder to see what's going if you are moving code as well as
changing it. If the other devs agree it needs to move to core, I'd prefer to
do two commits, the first which is just functionality changes - for the sake
of a comprehensible changelog - and the second that moves the files and
changes all the imports etc. To do this properly, we'd also need to
reorganise docs, fix all the :ref:s etc.
Another problem with moving it all is that is makes upgrade instructions a bit
more complex (which had just got a bit simpler), but I guess we may as well do
it all at once.
> I'm reasonably happy with the testing approach you've taken - or, at
> least, I can't think of anything substantially better.
>
>
> My only comment is that we might want to put an underscore on the
> magic variable (i.e., request._dont_enforce_csrf_checks) to reinforce
> the fact that this really isn't public API.
Good call, I'll fix that. I have mentioned the existence of the attribute in
the docs in the 'testing' section, but not its name, precisely because it's
not a public API, but some people might need to know about it in order to fix
their tests.
> At this point, I'm convinced, mod the minor things I've flagged.
> However, I'd like to see Jacob and Malcolm chime in before this is
> committed.
Agreed.
Luke
--
"Pessimism: Every dark cloud has a silver lining, but lightning kills
hundreds of people each year trying to find it." (despair.com)
Luke Plant || http://lukeplant.me.uk/
Luke Plant
> The main reason I really like preserving form data is that it means
> CSRF failures are less of a problem, allowing us to be much more
> strict about how they work (setting form tokens to expire after a few
> hours, tying tokens to individual forms etc). This means we can reduce
> the damage caused in the case of a token leaking somehow. This becomes
> particularly important if tokens are going to be used to protect GET
> requests, which some applications may want to do (Flickr have CSRF-
> protected GETs for their logout and change-language links, for
> example) - GET tokens are more likely to leak in other people's
> referrer logs or to intermediate proxies.
I'm not convinced that token leakage is going to be a problem that can easily
be fixed with timeouts. Having someone's CSRF token isn't going to be useful
in an attack unless you know whose it is. (An attacker could possibly log IP
addresses against CSRF tokens for later attacks, but that's not likely to be
very successful). The obvious and easy way to do any kind of systematic
attack is to immediately send some attack javascript back in response to any
incoming request that contains a Referer from an external target along with a
csrf token. Timeouts are not going to help you there.
> I'd be enormously happy to see that go in as django.shortcuts.render -
> the name may not be as descriptive as render_to_response but for
> something used in practically every view I think terseness should beat
> descriptiveness.
+1. Your TemplateResponse class sounds cool, but perhaps too clever to be
default way of doing things that we would put in documentation.
> 2. I'm not at all keen on the implementation as a middleware
> (especially a view middleware, which doesn't play well with generic
> views and redispatching to other view functions, both patterns I like
> a lot).
Could you explain a bit more about the difficulties with generic views?
AFAICS, decorators seem slightly worse than a middleware, because you end up
using them twice e.g.:
@csrf_protect
def some_generic_view(request, **kwargs):
# POST processing here.
@csrf_protect
def my_view(request):
# some stuff here, then
return some_generic_view(request, blah='blah')
With a middleware, you don't have this duplication. What are the
disadvantages of the middleware?
> I'd be perfectly happy with a decorator that is also available
> as a middleware.
As it stands, there is a middleware and a decorator created using
decorator_from_middleware.
> 3. I'd like to include support for CSRF form tokens to expire and be
> locked down to individual forms, as seen in SafeForm. Whether or not
> we actually go for that depends on how likely we are to find a
> solution to the next point...
> 4. I'd love it if we could find a way for developers who care (such as
> myself) to opt-in to CSRF-failing gracefully at the form validation
> level. I'm confident this should be possible, but I don't think it's
> necessary to make it the default behaviour (the CSRF 403 screen is
> probably fine for most people).
If I'm thinking correctly, this isn't too hard:
1) Implement your csrf_protect_form method. That could easily add your
requirement to lock down to individual forms and timeouts. It would need
cooperation from a new template tag, or additional optional arguments to the
current template tag. It might also need an additional context processor in
settings, but as an opt-in solution that's OK.
I think the solution that manipulates request.POST sounds OK - yes a hack, but
providing this method is not the default, most people won't have to deal with
any problems with it.
2) Get the view to be exempted from the normal CSRF checks done by the
middleware. Thankfully, we already have not one but two ways of doing this -
the manual @csrf_exempt decorator on views, and the internal mechanism that
allows the decorator and middleware to avoid duplicate checking.
Automatically doing the latter in csrf_protect_form is probably the way ahead.
So, if I'm thinking straight, it should just be a matter of this:
-- views.py ---
@csrf_protect_form(name="myform", timeout=60*60*24)
def my_view(request):
# ...
render(request, 'my_template.html', ...)
-- my_template.html --
<form method=POST>{% csrf_token "myform" %}
The only question mark in my mind is what happens with multiple forms on a
page (e.g. when you have a login box on every page). It might not be an issue
- the target of the login box will be another view anyway - but it needs
thought.
Simon Willison
> > 2. I'm not at all keen on the implementation as a middleware
> > (especially a view middleware, which doesn't play well with generic
> > views and redispatching to other view functions, both patterns I like
> > a lot).
>
> Could you explain a bit more about the difficulties with generic views?
that's not really what I meant either. My problem with view middleware
is that it makes it much harder to use the pattern where view
functions re-dispatch to other view functions. The classic example is
a site with a URL structure that looks like this:
/category/sub-category/sub-sub-category/arbitrarily-deep-nesting/news/
I've built stuff like this in the past by essentially discarding
urls.py - I instead map "^(.*)$" to my own view function which then
splits request.path on slashes and re-dispatches based on its own
custom logic. View middleware doesn't really fit that model - it
assumes that the top level URL configuration will resolve to the
callable that's actually going to implement the view, but that isn't
necessarily the case for complex sites.
A more common example is view functions which re-dispatch to other
functions:
def homepage_view(request):
if request.user.is_authenticated():
return logged_in_homepage_view(request)
else:
return default_homepage_view(request)
A view middleware will just see "homepage_view" - it won't know which
of the actual view functions is going to be called.
I'm a big fan of class-based generic views which can involve quite a
bit of subclassing and re-dispatching to other methods (like my
RestView class http://www.djangosnippets.org/snippets/1071/ ) which is
another reason view middleware makes me uncomfortable.
Note that this is a philosophical objection to process_view middleware
as a whole, not anything specific to CSRF.
> 1) Implement your csrf_protect_form method. That could easily add your
> requirement to lock down to individual forms and timeouts. It would need
> cooperation from a new template tag, or additional optional arguments to the
> current template tag. It might also need an additional context processor in
> settings, but as an opt-in solution that's OK.
>
> I think the solution that manipulates request.POST sounds OK - yes a hack, but
> providing this method is not the default, most people won't have to deal with
> any problems with it.
how it comes out.
> The only question mark in my mind is what happens with multiple forms on a
> page (e.g. when you have a login box on every page). It might not be an issue
> - the target of the login box will be another view anyway - but it needs
> thought.
this:
<form action="/login/" method="post><p>
<input type="text" name="username">
<input type="password" name="password">
<input type="submit" value="Log in">
{% csrf_token "login" %}
</p></form>
<form action="/rate/56/" method="post><p>
Your rating: <input type="text" name="rating">
<input type="submit" value="Submit rating">
{% csrf_token "rate" %}
</p></form>
Cheers,
Simon
Luke Plant
> 2) Get the view to be exempted from the normal CSRF checks done
> by the middleware. Thankfully, we already have not one but two
> ways of doing this - the manual @csrf_exempt decorator on views,
> and the internal mechanism that allows the decorator and
> middleware to avoid duplicate checking. Automatically doing the
> latter in csrf_protect_form is probably the way ahead.
Hmm, had a thinko there. The middleware is run *before* decorators
have had a chance to modify the request object. So only the first
of these will work I think. That plays badly with your method of
dispatching from your own view code. You will have to manually
csrf_exempt your top level view code, and manully apply
csrf_protect_form as needed.
Luke
--
"Pretension: The downside of being better than everyone else is
that people tend to assume you're pretentious." (despair.com)
Luke Plant || http://lukeplant.me.uk/