Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Luke Plant]

Hi all,

First, I want to say that complex things fail in complex ways, so there 
it's probably a fallacy to look for a single root cause. I agree with 
various other points about mistakes that were made, but not others. 
Particularly:


On 22/11/16 12:41, Florian Apolloner wrote:

Hi,

On Monday, November 21, 2016 at 11:56:56 PM UTC+1, Tim Graham wrote:

… that the existing tests would catch this type of obviously
incorrect issue.


I think that is the main issue here. I was also really surprised to 
discover that the tests were missing cases like this -- then again, 
writing good tests is hard and I know that I am personally one not to 
write good tests usually. Either way, I think the way forward is to 
improve the testsuite in this case.


I disagree with this, there was no issue with the existing test suite 
for the FBV version. *Tests are never close to exhaustive and we should 
never think they are or can be.* Tests are spot checks of correctness in 
an infinite universe of possible input values. When trying to work out 
what you should test in this vast space, there are a few sensible ways:


1. Look at the requirements, and pick spots that match each requirement. 
In this case:


 * If a request has the required token, it should be allowed through
 * If a request does not have the required token, it should not be
   allowed through

2. Make some guesses about well known edge cases and test them (e.g the 
empty string case etc.). However this could be a complete waste of time


3. Look at the implementation at work out where it might go wrong. This 
one is really important in reality.


And this is what the existing tests did. Out of the infinite number of 
HTTP requests to test under point 1, they happened to pick a GET 
request, which was perfectly sensible. In the case of FBVs, the control 
flow is so clear that all of the following are equally silly tests to add:


 * Check that requests with query string parameter 'xyz' doesn't
   introduce a back door
 * Check that on leap years tokens are checked
 * Check that the security is correct for requests that are
   specifically GET, POST, HEAD, OPTIONS, DELETE


The conclusion is this: in CBVs control flow is much, much less clear. 
In the FBV the mistake would have been immediately obvious, the CBV base 
classes obfuscated things to the point where it wasn't at all clear. To 
write adequate tests for the CBV version, under point 3 above, you have 
*much, much more work* to do.


I disagree that forcing people to write their own auth views is 
*necessarily* worse than providing a CBV that people can customise. 
Given this demonstration of how CBVs obfuscate control flow, it's quite 
possible that writing an FBV from scratch will be less error prone than 
using a CBV and subclassing, especially when we have encapsulated things 
like the token checking. Personally I would be massively happier 
auditing a custom FBV than a subclassed CBV, especially when you 
consider that the CBV subclass is inheriting from a stack of classes 
that could change in later versions of Django in some subtle way that 
breaks the code. Subclassing is not necessarily safer, it just feels 
safer ("I'm only changing this one little method"), and *that feeling of 
greater safety is itself a huge danger*.


Going forward, I think we need to:

1. Recognise that CBVs are much harder to reason about, and therefore 
require much more care.

2. Avoid using CBVs unless you really need them.
3. Recognise that tests for FBVs are inadequate against the CBV translation.
4. Not deprecate the FBVs. At the very least, they provide a sensible 
and easy to understand starting point for people that want to copy the 
logic to create their own, and do so in a way that they actually 
understand and can audit.


I'd be +0 on reverting to an FBV only, but it's part of a much bigger 
discussion


Regards,

Luke

--
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
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/e160c9a0-0af0-7767-4b35-cb02a92d6a4d%40cantab.net.
For more options, visit https://groups.google.com/d/optout.

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[João Sampaio]

+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 
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, 

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Josh Smeaton]

+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/passwordresetconfirmview-security-advisory/
>> [2] 

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[benjaoming]

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/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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 

Re: Newbie's questions

[Tim Graham]

You may use a PDF editor to sign the contributor license agreement rather 
than printing and scanning.

The contributing documentation describes the ticket triage process. If you 
have specific questions, please ask.

On Monday, November 21, 2016 at 12:34:53 PM UTC-5, ranvir singh wrote:
>
>
>
> On Saturday, November 19, 2016 at 3:44:11 AM UTC+5:30, Jeremy Spencer 
> wrote:
>>
>> There is extensive details on the django project website on this topic: 
>>
>>- https://docs.djangoproject.com/en/dev/internals/contributing/
>>- 
>>
>> https://docs.djangoproject.com/en/dev/internals/contributing/new-contributors/
>>
>> Thank you very much for your help, I already have read this. I want to 
> know the importance of contributors licence that the community want the 
> people to fill before contributing. Can't this process be automated as the 
> person have to take a print out and then send the signed copy to the 
> community. 
>
> Also I have been working with github issues for a while so it is a little 
> difficult to understand the ticket system and bug tracking system that the 
> community uses. Can you please shed some light on this too.
>
> --
> Ranvir Singh
> https://ranvirsinghprojects.wordpress.com
> https://github.com/singh1114
>   
>

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
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/77da7f25-582f-4491-9043-06d1fe3b2544%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Window expressions, #26608

[Mads Jensen]

Hi,

I got somewhat stuck on progress with this ticket, and as I'd like to
get it merged eventually (and avoid an abundant amount of fixes), I have
a few things I like a bit of input about.

1. Since this is specific to postgres, I'm looking for a better place to
put the actual Window-expression class, as well as axillary helpers
(Order By, Partition and the frame).
django/db/models/sql/expressions.sql may be one such place, but perhaps
introducing another file for this? Eventually, something like WITHIN
GROUP could be added, which shares some traits with window expressions.
This also goes the tests.

2. Tests, and if the initial test-cases seem fair? I took a small sample
of test cases related to some made-up salaries in a few departments. I
tried to introduce some wrapper to guard against testing functions that
aren't available (Oracle is the most complete in this sense, PostgreSQL
doesn't support all of the aggregate-functions, such as Variance).

3. The Order By-clause proposed in the initial code in the code only
leaves room for one column (no expressions). A regular query has
"add_ordering" to add more than one ordering-clause to a query, but this
just works for an ordering in SELECT, and I didn't spot anything that
could be used for this. Maybe abstracting the code from add_ordering so
it could be used also for something more? Something similar goes for
Partition By. Again, I'm sure it's a common use case to partition and
order by more than one expression. I currently commented out the
OrderByWrapper.

4. Would it be ok to add code for this in the base backend? I'm thinking
of constants for CURRENT ROW etc. which are shared among backends. I had
a look at SQLalchemy to see how things are arranged there, and what it
supports. SQLalchemy is feature-rich, but leaves out more
expressive-style ranges, and only supports unpreceding following/current
row, unbound following. I guess this is a reasonable limitation to make.

5. Will a 2.0 release notes be introduced soon, as I'm certain I won't
manage to make the final PR before the 1.11 feature freeze as it needs
to be finished, reviewed and merged. I started some time ago, and had it
lying around till I understood window functions better.

As pointed out in #26608, I have some rough, drafty code that's quite
far from a PR. I'm more interested in input for the above.

I'm relatively new to Django internals (it's fun and educational hacking
on them), and it took me some time to read through the code and
documentation for the ORM, and there are many facets of it I can still
understand. If someone is willing to guide me a bit, I'd be quite
helpful. Thank you.
-- 
Med venlig hilsen / Kind regards,
Mads Jensen

 Saajan Fernandes: I think we forget things if there is nobody to
   tell them.
 -- The Lunchbox (2013)




-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
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/482d2667-dea7-fd17-e820-6d6a0be08ae4%40inducks.org.
For more options, visit https://groups.google.com/d/optout.

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Erik Romijn]

Hello,

Thank you for raising this, Markus.

I am +1 to everything Baptiste said.

In particular, if our conclusion of this bug would be that CBV are entirely 
unsuitable for security sensitive features, I don’t think removing CBV for auth 
is enough. Because by that logic, our users will be making the same mistakes, 
and the logical step would be to remove CBV from Django entirely. Which is not 
something I would advocate.

If we are concerned about it being easier to cause vulnerabilities in CBV, I 
think updates to the documentation might be the best place to address this.

Erik

> On 22 Nov 2016, at 09:02, Baptiste Mispelon  wrote:
> 
> 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 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/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 django-developers+unsubscr...@googlegroups.com 
>> 

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Florian Apolloner]

Hi,

On Monday, November 21, 2016 at 11:56:56 PM UTC+1, Tim Graham wrote:
>
> … that the existing tests would catch this type of obviously incorrect 
> issue.
>

I think that is the main issue here. I was also really surprised to 
discover that the tests were missing cases like this -- then again, writing 
good tests is hard and I know that I am personally one not to write good 
tests usually. Either way, I think the way forward is to improve the 
testsuite in this case. 

What worries me really though is that if we make this kind of simple but 
horrible mistakes how are we going to prevent (especially) newcomers from 
making the same mistakes?

Cheers,
Florian

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
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/6197a4a0-f056-4ba3-a1c7-0a4079fc190e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Baptiste Mispelon]

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 django-developers+unsubscr...@googlegroups.com 
.
To post to this group, send email to 
django-developers@googlegroups.com 
.

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 
.

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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 

Re: Consider reverting or adding guidelines on how to use class based views for security sensitive features

[Claude Paroz]

Le lundi 21 novembre 2016 23:56:56 UTC+1, Tim Graham a écrit :
>
> When reviewing the patch for the auth class-based view additions, I made 
> the mistake of assuming that the existing tests would catch this type of 
> obviously incorrect issue. Perhaps with the function-based views, the code 
> was "too obviously correct" that a test for token verification on POST 
> wasn't considered. I'd like to think that future work on the class-based 
> views would be more careful about testing, especially after we made this 
> mistake.
>

+1 to that, I think this issue essentially demonstrated a weakness in the 
test suite.
 
More generally and considering the patch entered the master branch soon in 
the schedule, it would be a shame not being able to make it stable and 
secure for 1.11. I could understand the delay of the function-based view 
deprecation, but I would be very disappointed to see that code removed for 
1.11.

Also notice I don't buy the argument of a "danger of shooting ourselves in 
the foot". Currently, when people have to customize some part of the 
current views, they have no choice beside copy-pasting large part of the 
Django code in their code base. As versions evolve, some parts become stale 
and could even miss patches for security issues. So for me, by using 
class-based views and allowing users to only override the precise part of 
those views which needs to be overriden, the design is globally more secure 
than the current function-based views.

And of course, sorry for being the author of the faulty commit, and thanks 
to the eagle eyes!

Claude

-- 
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 django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
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/6e0c9d43-c6cb-48f3-bf07-8f1ff5ef625c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.