+1 to Baptiste, Ben and Josh. Especially as Ben said, I think this does not justify removing the auth CBVs. The bug happened because of a *misuse* of the abstractions involved. ``.get_context_data()`` is not the correct place for a security check (as nobody overriding it would expect that, nor from the name or documentation of the method, nor from the pattern established by the other CBVs), and neglecting this is one of the causes of the bug. Another cause of the bug is the missing test case, which is also unrelated to the CBV.
I also agree that removing them would decreased security. When CBVs are well used, they are much stronger than FBVs, in which they allow reuse of reviewed code. They also make writing more modular code easier. (I've seen FBVs with hundreds of lines, while CBVs more often have code split in meaningful methods.) The answer is simply to face this as something that happened to ring a loud bell: maybe security features should be more thoroughly reviewed, requiring maybe 2 or 3 reviewers to approve before being merged. Maybe we can even define a rule on "probation period", that new security-related features must sit in the master branch for 6+ months before being released (to avoid a security feature landing on master and being in a release a month later). This could have happened in this case, luckily it was caught before it was released. We can increase the chances of this happening more times. On Tue, Nov 22, 2016 at 7:54 PM, Josh Smeaton <[email protected]> wrote: > +1 to everything Baptiste and Ben have said. A bug in a CBV isn't a good > argument for throwing away CBVs entirely. We should probably review patches > that touch security systems quite a bit more thoroughly in the future - > meaning more eyes rather than a single set of eyes spending more time. > > > On Wednesday, 23 November 2016 03:06:42 UTC+11, benjaoming wrote: >> >> As a big fan of GCBVs, this got me out of the chair :) I am -1 for >> removing GCBVs for authentication. >> >> My reasons: >> >> 1) Security improvements also happen over time, finding a security hole >> in a component is not a reason to point the finger at the architecture. If >> you entirely remove something, you will have to rebuild it somewhere else, >> and new security holes will emerge. Thinking that users of builtin GCBVs >> will just opt for the FBVs is wrong, we won't, we'll write our own GCBVs or >> find some third-party that has less attentive eyes than the >> django.contrib.auth >> >> 2) In this case, having security checks in `get_context_data()` is far >> from the intentions of that method. If I override it, I would not expect >> security properties of the view to be removed, right? We shouldn't put >> security checks there, simple as that. If we assume the design pattern is >> (G)CBV, then this is a violation of the design principles and should have >> been rejected even before the security issue happened. In a GCBV, we should >> always ask "Is this logic to be expected by someone inheriting from this >> GCBV". >> >> 3) People have implemented and deployed GCBV from auth. The consequence >> of removing them because we don't want to deal with their security issues >> is alarming! I inherit from these classes, believing that we will be >> stronger together, because inheriting from a common codebase will have more >> eyes on the security aspects. This will be a bad example or bad signal to >> set for other usages of GCBVs. >> >> 4) SomeViewClass.as_view() generates an FBV, I'm sure we can bridge CBVs >> and FBVs if the concern is about having separate codebases as Baptiste >> expressed it. >> >> 5) I fully agree with Baptiste: If Django does not supply easily >> pluggable authentication patterns, someone else will (and not just 1 >> project, many will popup, they already have), and the effort will be >> fragmented, the security will be weakened. >> >> 6) I write CBVs in my own apps, I know there is a lot of afterthought and >> you often have to move logic between CBVs, mixins, and method decorators. I >> fully understand and sympathize with release notes that have entries about >> design changes and small refactors in GCBVs, that's awesome and you can >> learn from the considerations and decisions in Django's GCBVs. >> >> >> Thanks :) >> Ben >> >> >> On Monday, November 21, 2016 at 8:11:34 PM UTC+1, 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 the thread 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/passwor >>> dresetconfirmview-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]. > 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/9a031420-c8e9-407f-a32c- > 8a14e47a0d98%40googlegroups.com > <https://groups.google.com/d/msgid/django-developers/9a031420-c8e9-407f-a32c-8a14e47a0d98%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/CAO_YKa3nw2pCuV3qOft6FpNcE_RAhaLkQpNuwUJiy9pVO_krig%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
