Hi Markus,
Thanks for your clear description and for bringing this up for discussion.
I don't agree with your conclusions though.
1) Keeping around two implementations of auth views seems
counter-productive to me in terms of security because it effectively
doubles the potential for bugs or security issues (not to mention the
added maintenance work). We should have only one set of auth views.
2) Our users want more extension hooks for auth views. If we don't
provide them, some users will end up copy/pasting Django's views and
tweak what they need (I have done that myself more than once). This
copy/pasted code then won't receive updates (potentially
security-related) when Django changes, which also presents a danger to
our users (unless they track changes in the code, which is unlikely).
3) Your description of the security issues seems quite alarming but how
does it compare with others in the past? To me, a 4-month window
actually seems quite small, not to mention that the issue never even
made it to a release.
4) I agree with Tim that there's an issue in our test suite.
Function-based views give you the assumption that all HTTP methods will
use the same entry point into your view. You lose this assumption with
class-based views but I don't view this as a defect: to me, this is one
of the big advantages of class-based views. This probably means that we
should audit Django's code and add tests to make sure we cover all
supported HTTP methods.
Thanks,
Baptiste
On 11/21/2016 08:11 PM, Markus Holtermann wrote:
Hi all,
As it turned out [1], due to their complexity, using class-based
generic views for security-sensitive functionality can result in
unintended behavior. Essentially, the reset token was only checked on
GET requests, not on POST. This was due to the check being in
`get_context_data()` (which is only called on GET but not POST except
for invalid forms) and not higher up the stack. Validation could
happen in the SetPasswordForm but doesn't really belong there either.
The form is being used by the admin to allow superusers to change
other users' password. Also, password resets could probably happen via
other ways that want to leverage a form that doesn't require a token.
In the end, from my perspective the check for the correct token does
belong in the view.
While the reported issue was fixed [2] it raises the question if the
added functionality of class-based generic views is worth the danger
of shooting ourselves in the foot. I see the benefits of GCBVs. But
given that the reported issue stayed unnoticed for 4 months makes me
think that those views are not the best for these use cases and easily
underpin the security functionality. Hence I suggest to revert the
patch (including all additional features they gained) unless they are
integrated in the function-based views and add guidelines on how to
use class-based generic views for security sensitive feature.
This is thethread to get the discussion about this started.
One thing I want to suggest regardless if the class-based generic
views are removed again or not, is to hold off the deprecation of the
function-based views. This allows users who feel the same to not use
class-based generic views without having deprecation warnings. At
least until the next LTS release.
Furthermore, myself and Florian Apolloner, who discovered the issue,
are leaning +0 to +1 on the revert of the class-based views.
Cheers,
Markus Holtermann
[1]
https://www.djangoproject.com/weblog/2016/nov/21/passwordresetconfirmview-security-advisory/
[2] https://github.com/django/django/pull/7591
--
You received this message because you are subscribed to the Google
Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected]
<mailto:[email protected]>.
To post to this group, send email to
[email protected]
<mailto:[email protected]>.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/19b675f5-c25a-43e8-ac73-2a31b9e351d6%40googlegroups.com
<https://groups.google.com/d/msgid/django-developers/19b675f5-c25a-43e8-ac73-2a31b9e351d6%40googlegroups.com?utm_medium=email&utm_source=footer>.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups "Django
developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/32ca3ed6-abbe-1268-bd12-9cce3c314d9f%40gmail.com.
For more options, visit https://groups.google.com/d/optout.
