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 :-)
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/
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
