On Sun, Sep 20, 2009 at 2:57 AM, Luke Plant <l.plant...@cantab.net> wrote:
>
> On Saturday 19 September 2009 16:56:52 Russell Keith-Magee wrote:
>
>> On Fri, Sep 18, 2009 at 6:09 AM, Luke Plant <l.plant...@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 %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---
