Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Torsten Bronger
Hallöchen!

Alex Ogier writes:

> [...]
>
> That suggests an idea to me. Perhaps the best way to check this
> isn't on the way out in the template renderer, but rather on the
> way back in in the form validation. If the form doesn't get back
> exactly those fields it sent out then you know that for whatever
> reason, the field was unable to make a round trip.

But can one guarantee that fields rendered in the browser are also
sent back in the POST request?  Even worse, how about non-browser
requests?

Tschö,
Torsten.

-- 
Torsten BrongerJabber ID: torsten.bron...@jabber.rwth-aachen.de
  or http://bronger-jmp.appspot.com

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Anssi Kääriäinen
On 14 kesä, 03:12, Doug Blank  wrote:
> I, too, was thinking about this kind of solution. In fact, it came up
> for me the other day because I had forgotten to exclude a field that I
> did not have on the form, and so the value ended up getting wiped out
> when I saved. So, perhaps a solution that prevented others from adding
> fields could also be a solution that checked to make sure that the
> form was editing all fields it should be.

I hadn't realized all fields present on the Python form but not in the
HTML form will get overwritten to NULL... The above makes me a tad
more "-" on the require fields always proposal. The problem is there
only for forms used for model creation. If you use the form both for
update and creation, you will soon enough see that something funny is
going on as part of your fields are getting set to NULL on
form.save().

So, to hit the problem the user would need to:
  1. Have a ModelForm with no field restrictions in Python.
  2. Render only part of the fields.
  3. All non-rendered fields must be null/blank=True for the form to
work at all.
  4. Not use the form in update views.

(there are of course corner cases - the model has "deleted" attribute
which is usually null - you don't include that in the HTML form - the
user can update the delete field and it doesn't show up in testing)

This seems to be different from what Rails do: they have
update_attributes which updates all model attributes present in the
request, but lefts all others untouched. So, in Rails if you render
only part of the fields in update view, then you will not get the non-
included fields overwritten to NULL, which conveniently hides the
problem that the fields are in fact editable through the request.

To me it seems there is no similar attack vector against Django's
implementation as there were against Rails.

I hope my quick investigations are correct - double checking what
Rails do and the four conditions above is suggested...

 - Anssi

-- 
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.



Ticket #18471: Add a localization field option in Model Fields.

2012-06-13 Thread Serge G. Spaolonzi
Hi, 

I have opened the ticket #18471 to 
request the inclusion of a new option for localization to the Model Fields. 
Currently the localization of the Model Fields is a complex process that 
involves the utilization of ModelForms, it can be simplified to the 
inclusion of a single parameter.

I have a branch in Github with this function implemented:
https://github.com/cobalys/django/tree/ticket_18471

The only modified file is: django/django/db/models/fields/__init__.py

I would like to know what are the next steps to request the inclusion of my 
code. Should I post a pull request from Github?

Thanks
--
Serge G. Spaolonzi
Cobalys Systems - Web Development | Django  | jQuery
se...@cobalys.com · +(598) 96492712 · http://www.cobalys.com

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To view this discussion on the web visit 
https://groups.google.com/d/msg/django-developers/-/o-k9mfPY2sEJ.
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Atul Bhouraskar
Hi All,

I can see two separate (but related) problems that need to be solved:

1. Ensure that sensitive fields are not accidentally exposed to users (e.g
when a database model is updated to include one), and
2. Provide a means to confirm whether the fields as rendered and submitted
exactly match the intent of the form.

This is my proposal for problem 1:

Add an "is_sensitive" or similarly named boolean attribute to model fields.
This field would be False by default and would be set to True for fields
that contain sensitive data e.g:

class Article(models.Model):
  text = models.TextField()
  secret_notes = models.TextField(is_sensitive=True)

Now a ModelForm would by default exclude any field that is marked as
sensitive, unless it is explicitly included via the "fields" attribute.

The advantage of the above is that it allows mechanisms other than Forms to
enforce rules associated with sensitive data and could potentially be
double checked while being rendered as well. For developers who don't care
about sensitive data, nothing changes, unless the is_sensitive attribute is
made True by default (which would be a pain).

Regards,


Atul

On 14 June 2012 10:18, Carl Meyer  wrote:

> On 06/13/2012 06:12 PM, Doug Blank wrote:
> > On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyer  wrote:
> >> On 06/13/2012 02:55 PM, Peter wrote:
> >>> Can I throw in option 5:
> >>>
> >>> We leave ModelForms as they are, but emit a warning if you only
> >>> partially render the form?
> >>>
> >>> I'm not sure how feasible this is, but presumably we could keep track
> of
> >>> which fields have been rendered for a given form instance?
> >>>
> >>> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
> >>> your new sensitive field appear in the page. If you manually render the
> >>> form, you'll get a warning.
> >>
> >> I've thought about this. The main problem is that the implementation is
> >> quite difficult in practice: at what point do you perform the check?
> >> There isn't any such thing as an "ok, I think I'm all done rendering
> >> this form now, tell me if I did it right" hook.
> >
> > I, too, was thinking about this kind of solution. In fact, it came up
> > for me the other day because I had forgotten to exclude a field that I
> > did not have on the form, and so the value ended up getting wiped out
> > when I saved. So, perhaps a solution that prevented others from adding
> > fields could also be a solution that checked to make sure that the
> > form was editing all fields it should be.
> >
> > What about a {% validate %} tag in the form which would do a runtime
> > check to make sure that all non-excluded fields had been rendered?
>
> Yeah, this would be a cleaner way to implement the check. I'd like to
> see it proved out as a third-party add-on before discussing it for core.
> One of the unresolved issues in my mind is what it should actually _do_
> if you haven't rendered all the fields on the form (blow up in DEBUG
> mode only? Just a call to warnings.warn?).
>
> And since it's opt-in (and easy to forget or not bother with) I'm not
> sure that by itself it's a satisfactory solution to the original problem
> of implicit Meta.fields.
>
> Carl
>
> --
> 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.
>
>

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Alex Ogier
On Jun 13, 2012 8:12 PM, "Doug Blank"  wrote:
>
> I, too, was thinking about this kind of solution. In fact, it came up
> for me the other day because I had forgotten to exclude a field that I
> did not have on the form, and so the value ended up getting wiped out
> when I saved. So, perhaps a solution that prevented others from adding
> fields could also be a solution that checked to make sure that the
> form was editing all fields it should be.
>

That suggests an idea to me. Perhaps the best way to check this isn't on
the way out in the template renderer, but rather on the way back in in the
form validation. If the form doesn't get back exactly those fields it sent
out then you know that for whatever reason, the field was unable to make a
round trip. In a ModelForm with implicit fields this is the root cause of
this whole security dilemma.

This solution is parsimonious because it's easy to explain: "If a form
didn't get back all the fields it expected then there was probably an error
of omission rendering it. This causes a security hole where an attacker can
modify fields the developer doesn't expect, for further examples see "

It's an easy thing to justify turning on in an opt-out fashion,
Meta.allow_partial_submissions or something.

Best,
Alex Ogier

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Carl Meyer
On 06/13/2012 06:12 PM, Doug Blank wrote:
> On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyer  wrote:
>> On 06/13/2012 02:55 PM, Peter wrote:
>>> Can I throw in option 5:
>>>
>>> We leave ModelForms as they are, but emit a warning if you only
>>> partially render the form?
>>>
>>> I'm not sure how feasible this is, but presumably we could keep track of
>>> which fields have been rendered for a given form instance?
>>>
>>> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
>>> your new sensitive field appear in the page. If you manually render the
>>> form, you'll get a warning.
>>
>> I've thought about this. The main problem is that the implementation is
>> quite difficult in practice: at what point do you perform the check?
>> There isn't any such thing as an "ok, I think I'm all done rendering
>> this form now, tell me if I did it right" hook.
> 
> I, too, was thinking about this kind of solution. In fact, it came up
> for me the other day because I had forgotten to exclude a field that I
> did not have on the form, and so the value ended up getting wiped out
> when I saved. So, perhaps a solution that prevented others from adding
> fields could also be a solution that checked to make sure that the
> form was editing all fields it should be.
> 
> What about a {% validate %} tag in the form which would do a runtime
> check to make sure that all non-excluded fields had been rendered?

Yeah, this would be a cleaner way to implement the check. I'd like to
see it proved out as a third-party add-on before discussing it for core.
One of the unresolved issues in my mind is what it should actually _do_
if you haven't rendered all the fields on the form (blow up in DEBUG
mode only? Just a call to warnings.warn?).

And since it's opt-in (and easy to forget or not bother with) I'm not
sure that by itself it's a satisfactory solution to the original problem
of implicit Meta.fields.

Carl

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Doug Blank
On Wed, Jun 13, 2012 at 5:05 PM, Carl Meyer  wrote:
> Hi Peter,
>
> On 06/13/2012 02:55 PM, Peter wrote:
>> Can I throw in option 5:
>>
>> We leave ModelForms as they are, but emit a warning if you only
>> partially render the form?
>>
>> I'm not sure how feasible this is, but presumably we could keep track of
>> which fields have been rendered for a given form instance?
>>
>> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
>> your new sensitive field appear in the page. If you manually render the
>> form, you'll get a warning.
>
> I've thought about this. The main problem is that the implementation is
> quite difficult in practice: at what point do you perform the check?
> There isn't any such thing as an "ok, I think I'm all done rendering
> this form now, tell me if I did it right" hook.

I, too, was thinking about this kind of solution. In fact, it came up
for me the other day because I had forgotten to exclude a field that I
did not have on the form, and so the value ended up getting wiped out
when I saved. So, perhaps a solution that prevented others from adding
fields could also be a solution that checked to make sure that the
form was editing all fields it should be.

What about a {% validate %} tag in the form which would do a runtime
check to make sure that all non-excluded fields had been rendered?

-Doug

> There's at least one third-party app out there that does this
> (https://github.com/ulope/django-careful-forms), but it registers all
> forms in a thread-local and performs the check in a middleware; that's
> not something I think belongs in core Django.
>
>> One problem would be excessive warnings if you went further and hand
>> craft the HTML - does anyone do that?
>
> Yes.
>
> Carl
>
> --
> 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.
>

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Carl Meyer
Hi Peter,

On 06/13/2012 02:55 PM, Peter wrote:
> Can I throw in option 5:
> 
> We leave ModelForms as they are, but emit a warning if you only
> partially render the form?
> 
> I'm not sure how feasible this is, but presumably we could keep track of
> which fields have been rendered for a given form instance?
> 
> That way, if you render the whole form ( {{ form.as_p }} ) you'll see
> your new sensitive field appear in the page. If you manually render the
> form, you'll get a warning.

I've thought about this. The main problem is that the implementation is
quite difficult in practice: at what point do you perform the check?
There isn't any such thing as an "ok, I think I'm all done rendering
this form now, tell me if I did it right" hook.

There's at least one third-party app out there that does this
(https://github.com/ulope/django-careful-forms), but it registers all
forms in a thread-local and performs the check in a middleware; that's
not something I think belongs in core Django.

> One problem would be excessive warnings if you went further and hand
> craft the HTML - does anyone do that?

Yes.

Carl

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Peter
Hi all,

Can I throw in option 5:

We leave ModelForms as they are, but emit a warning if you only partially 
render the form?

I'm not sure how feasible this is, but presumably we could keep track of 
which fields have been rendered for a given form instance?

That way, if you render the whole form ( {{ form.as_p }} ) you'll see your 
new sensitive field appear in the page. If you manually render the form, 
you'll get a warning.

One problem would be excessive warnings if you went further and hand craft 
the HTML - does anyone do that?

Cheers,

Peter

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To view this discussion on the web visit 
https://groups.google.com/d/msg/django-developers/-/J74yecS0r5sJ.
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.



Re: QuerySet refactoring

2012-06-13 Thread Andrew Godwin
On 13/06/12 17:15, Luke Plant wrote:

> I think this is a very necessary piece of work. The problem with that
> layer of code is that it is very difficult to really grok and therefore
> to review patches. It would take almost as much effort to do a review of
> a substantial patch as the patch itself. You have to really understand
> what Query is doing, and it is a huge class, that takes hours to even
> vaguely get the hang of and get in your head.
> 
> I would like to be able to help, and if I dropped everything else I
> might otherwise do on Django, that should be possible. I think with two
> pairs of eyes on these changes, and with a good test suite, we should be
> reasonably safe, but getting more review than that would be hard.

I'm happy to look over any patches as well - while I generally know more
about the actual backends rather than Query, the more eyes the better on
this kind of stuff, I think.

> I do actually have another proposal, that has been a growing draft email
> on my computer for a few weeks: that we rewrite our backend using
> SQLAlchemy Core (not the ORM). I've used SQLAlchemy a bit, and I'm very
> confident we could use it profitably. While the ORM works significantly
> differently to ours, the lower layer (Core - the SQL Expression
> Language) can indeed build queries that can do all the joins etc that we
> would need - it's a full wrapper around SQL. Some of the complexity in
> our code comes from not having this wrapper.
> 
> SQLAlchemy is a really excellent library, and I fear that long term if
> we don't switch to it, we will just produce an ad-hoc, informally
> specified, buggy, slow implementation of half of SQLAlchemy.

I'd love to see the full proposal for this - it's been a while since
I've heard rumblings about SQLAlchemy, and I've forgotten what the major
objections/drawbacks were.

It would certainly save me some work if I could rely on it for some DDL
code generation, though because of the way models are defined in terms
of Fields that's unlikely to be possible soon without some serious
mapping and wrapping.

Andrew

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Carl Meyer
On 06/13/2012 12:57 PM, Anssi Kääriäinen wrote:
> The point is the same form with the same fields can be used in
> multiple places. There is no need for dynamic addition and removal of
> fields in Python code. The dynamism is in which fields to display -
> not which fields the form contains. You render only field1, except for
> superusers you render field1 and field2. Thus, the fields = ('field1',
> 'field2') in the form's meta doesn't do the security restriction you
> want for this case - the place to do the restriction is in the form
> init call:
> 
> if user is superuser:
> form = MyModelForm(request.POST, allowed_fields=('field1',
> 'field2'))
> else:
> form = MyModelForm(request.POST, allowed_fields=('field1',))

Ah, I see better what you're getting at now. I still think that using
the same form for two purposes in this way is an unusual case, and
probably not the best approach. I've never done it myself that I can
recall, I'd use two different ModelForm subclasses, probably one
subclassed from the other.

If someone is using the same ModelForm for both a superuser and a
regular user, I think it's pretty reasonable to expect them to realize
that that leads to some potential security issues, and do the
appropriate checks themselves.

IOW, unlike implicit Meta.fields, this doesn't seem to me like a case
where Django's defaults are actively leading them astray.

> Part of the problem is that it is easy to think that if you don't have
> the fields in the HTML form, then the user can't edit the fields.

Yes, this is really at the root of the whole issue. I don't see a good
way to address it directly - but we can address it indirectly by
ensuring developers have to think explicitly about which fields their
ModelForm is going to allow through.

Carl

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread David Danier
Hi list,

as I use "exclude" most of the times (to hide sensible fields from the
user) I don't like the idea of this beeing removed. Perhaps another
solution might be to force developers to either define fields _or_
exclude, so you have to at least think about this..without going through
too much trouble. Minimal form containing all fields would be:

class SomeModelForm(forms.ModelForm):
  class Meta:
exclude = []

Everything else would lead to develpers using thinks like mentioned
before ([f.name for f in MyModel._meta.fields if f.name not in ...]). I
really think this only makes things more ugly.

David


-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Anssi Kääriäinen
On 13 kesä, 20:58, Carl Meyer  wrote:
> > 2. The fix doesn't actually fix the issue for all cases. You are still
> > allowed to use the same form in a way that renders additional fields
> > based on user permissions or if the user is authenticated. If you
> > reuse the same form in multiple views for example, or your template
> > contains some logic to add/remove fields dynamically.
>
> I don't see how this is relevant. Dynamic addition or removal of fields
> is an uncommon edge case; it's way beyond the "insecure defaults" issue
> and well into "if you're doing advanced stuff, you just have to know
> what you're doing" territory.

The point is the same form with the same fields can be used in
multiple places. There is no need for dynamic addition and removal of
fields in Python code. The dynamism is in which fields to display -
not which fields the form contains. You render only field1, except for
superusers you render field1 and field2. Thus, the fields = ('field1',
'field2') in the form's meta doesn't do the security restriction you
want for this case - the place to do the restriction is in the form
init call:

if user is superuser:
form = MyModelForm(request.POST, allowed_fields=('field1',
'field2'))
else:
form = MyModelForm(request.POST, allowed_fields=('field1',))

Part of the problem is that it is easy to think that if you don't have
the fields in the HTML form, then the user can't edit the fields.

It would be useful to know how many projects contain ModelForms with
no field restrictions, and only display a subset and always the same
subset of the fields.

 - Anssi

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Alex Ogier
On Tue, Jun 12, 2012 at 7:16 PM, Luke Plant  wrote:
>
> = Option 1: Leave things as they are, just document the problem. This
> was the least popular view on django-core.
>
> = Option 2: Deprecate Meta.exclude, but still allow a missing
> Meta.fields attribute to indicate that all fields should be editable.
>
> The policy here is essentially this: if any fields the model are
> sensitive, assume all are potentially, and require whitelisting, not
> blacklisting.
>
> = Option 3: Make the 'Meta.fields' attribute a required attribute, which
> must list all the model fields that should be editable. Without it,
> ModelForms would not work at all.
>
> This also means deprecating Meta.exclude (it's redundant once you are
> saying that all fields should be explicitly whitelisted).
>

What about a fourth option:

= Option 4: Make the 'Meta.fields' attribute required, but allow a
semaphore value ("__all__"?) that enables the insecure shortcut. We
can plaster it with warnings in the docs, which people will inevitably
check when the error pops up, "Meta.fields is required." There are
clearly some use cases where the maintenance burden of whitelisting
all fields is just not worth it and people will resort to hacks if we
hamstring the current behavior. It also gives an upgrade path for
people who would otherwise need to rearchitecture their applications
if the Meta.fields attribute suddenly became required. (I can imagine
models with hundreds of fields, or worse, dynamically generated
fields.)

Like Carl Meyer, I don't think Django's goals should be to protect
developers from themselves, but I agree that it is important that the
path of least resistance be secure in all areas. There is considerable
value in "A form that is automagically scaffolded from a Model" above
and beyond "A form that does default
model-field-type-to-form-field-type mappings" which is what ModelForms
will become under options 2 and 3. So long as that is an explicit
choice, I don't think there is too much harm in letting people do that
if they want to. In fact, the reason I consider Option 1 viable is
precisely because I consider the creation of a ModelForm to be
precisely that kind of choice, but I can see how having the default be
include-all is a problem.

In summary:

+0 on option 1. Making a ModelForm is already a choice to expose a
model directly.
-1 on option 2. Default is still insecure, and Meta.exclude has use cases.
+0 on option 3. It's like my option 4, but requires [for field in
Model._meta.fields] hackery to work around.
+1 on option 4. Auto-generated fields are useful, and this allows an
explicit opt-in.

Best,
Alex Ogier

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Carl Meyer
Hi Luke,

Thanks for identifying this issue and continuing to push for a resolution.

On 06/12/2012 05:16 PM, Luke Plant wrote:
> == Option 1: This is the least secure, but most convenient - you have
> several ways to specify which fields should be editable, you can use
> whichever you like. "We're all consenting adults here".
> 
> An argument in favour of keeping this is if we don't, people will just
> use 'fields = [f.name for f in MyModel._meta.fields]' anyway.
> 
> == Option 2: the retains some of the convenience of option 1, while
> encouraging more careful handling of "sensitive" models.
> 
> == Option 3: the most secure, the least convenient. You have to list all
> fields for every ModelForm (except for cases of sub-classing forms,
> where the base class's Meta.fields would still work, of course).
> "Explicit is better than implicit".
> 
> The option doesn't make an assumption that models are either 'sensitive'
> or not. It is also more consistent than option 2: if you add a field to
> a model, you know that if it is meant to be publicly editable, you have
> to edit the ModelForms to add it, and if it is not meant to be editable,
> you don't, because the list is always "opt in".

I favor option 3; I think the Rails issue should have taught us that
exposing database data to modification by remote users is something that
deserves to always be done explicitly.

I think the most pressing issue is the behavior of UpdateView, because
it allows a naive developer to bypass the forms layer entirely (since it
automatically constructs a ModelForm subclass if given a model). As
others have pointed out, our situation in general is somewhat better
than Rails because we have the explicit Form layer; even if a developer
constructs a ModelForm with implicit fields they at least have to think
about the fact that this form should be responsible for data validation.
But this is not true with UpdateView; I think UpdateView is actually a
very close parallel to the Rails issue.

So while I favor option 3 for ModelForms themselves, I also think that
even if this discussion should end up rejecting that, we should still at
the very least require an explicit "fields" parameter for UpdateView.

Carl

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Carl Meyer
Hi Anssi,

On 06/13/2012 12:08 AM, Anssi Kääriäinen wrote:
> On 13 kesä, 02:16, Luke Plant  wrote:
>> For the record, I brought up the issue initially, and favour the most
>> secure of the options outlined below, but I'll try to summarise with as
>> little bias as I can!
> 
>> = Option 1: Leave things as they are, just document the problem. This
>> was the least popular view on django-core.
>>
>> = Option 2: Deprecate Meta.exclude, but still allow a missing
>> Meta.fields attribute to indicate that all fields should be editable.
>>
>> The policy here is essentially this: if any fields the model are
>> sensitive, assume all are potentially, and require whitelisting, not
>> blacklisting.
>>
>> = Option 3: Make the 'Meta.fields' attribute a required attribute, which
>> must list all the model fields that should be editable. Without it,
>> ModelForms would not work at all.
> 
> I support option 2. I will try to keep the reasoning part short. The
> end of post is devoted to another idea.
> 
> Here are the reasons why I think Option 3 isn't worth the trouble:
> 
> 1. Users will learn to use fields = [f.attname for f in
> Permission._meta.fields]. At that point we haven't gained anything in
> security, but made modelforms less convenient to use.

No, even if some users do this we will still have gained quite a lot.
There will always be some way for users to do whatever they like - this
is Python, and that's a good thing. But it's critically different when
the user must actively choose to circumvent a secure default, as opposed
to Django making a risky default the obvious path for naive newbies.

In any case, I think in actual fact only a small percentage of users
will do this; for many ModelForms just specifying the desired fields
will be simpler and easier.

> 2. The fix doesn't actually fix the issue for all cases. You are still
> allowed to use the same form in a way that renders additional fields
> based on user permissions or if the user is authenticated. If you
> reuse the same form in multiple views for example, or your template
> contains some logic to add/remove fields dynamically.

I don't see how this is relevant. Dynamic addition or removal of fields
is an uncommon edge case; it's way beyond the "insecure defaults" issue
and well into "if you're doing advanced stuff, you just have to know
what you're doing" territory.

> Yes, option 3 is a bit more secure than option 2, but I don't find the
> cost-benefit ratio correct. It will still not protect users from this
> admittedly easy to do security mistake. So, I am -0 to option 3.

Option 3 is the only option that addresses the real problem, which is
that the simplest and easiest way for a naive newbie to create a
ModelForm sets them up for trouble.

Asking for a solution that "protects users" is the wrong goal to begin
with - there's always a way to be insecure if you want. Our
responsibility as a framework is not to protect users, it's just to stop
actively encouraging them to do the wrong thing.

Carl

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Michael
I personally think that a job of Django is to stop naive developers from
doing stupid things. This is one of those things that a developer should
understand, but it is substantially hidden and a real problem that bots
exploit on the web. For this reason, option 3 would be my choice, but I
think we should start looking into a default "what's expected" scheme for
forms, because it does not really solve all of these problems.

I agree that option 3 will be white noise for developers who copy paste
code conventions, but it would at least give us a clear chance to document
why that decision was made. Could be a learning experience and for those
who don't read the documentation, it will be an annoyance.

Option 2 is concerning to me as well, because I think that a generic
ModelForm would be most likely exploited in the current set up. One
apparently easy (and DRY) way to provide granular permissions to users is
to do it inside of templates (especially for a developer coming from the
front end or PHP) by putting if statements around form fields [1]. I think
this is where this is most likely going to happen and while option 3
doesn't solve this problem completely, it does at least give us a chance to
explain why to developers.

While this problem is different from Rails, it is still very possible that
it could happen to Django in a similar way.


1.) https://docs.djangoproject.com/en/dev/topics/auth/#id9




On Wed, Jun 13, 2012 at 10:58 AM, Daniel Sokolowski <
daniel.sokolow...@klinsight.com> wrote:

> (I apologize if this is a duplicate - my other account appears to have an
> issue sending to this group)
>
> I'm in the camp don't change it.  ModelForms are doing what they are
> designed to do: providing a very handy short cut. It is a design decision
> to either use a ModelForm, or a a plain Form not tied to a model. If I want
> front end user functionality I use a Form, if I want admin front end
> functionality I use a ModelForm - I use a different approach based on the
> expected level of trust here.  The un-desired effect would also only come
> into play if these new fields were not required so they would need to pass
> validation - yes?  Actually I would suggest to remove 'fields' and leave
> the 'exclude' option, if I remove one two or three fields I might still
> have functioning ModelForm where I don't have to override save() to pass DB
> validation , any more excludes and a customized Form might be a better
> approach;  having the 'fields' option implies to me I can get away using
> ModelForm instead of Form and have falsely in past tried to do so.
>
> -Original Message- From: Luke Plant
> Sent: Tuesday, June 12, 2012 7:16 PM
> To: django-developers@**googlegroups.com
> Subject: ModelForms and the Rails input handling vulnerability
>
>
> Hi all,
>
> On django-core we've been discussing an issue that was security related.
> However, we decided it wasn't urgent enough to warrant a security
> release, and so we're moving the discussion here (especially because we
> couldn't agree on what to do).
>
> For the record, I brought up the issue initially, and favour the most
> secure of the options outlined below, but I'll try to summarise with as
> little bias as I can!
>
> The issue was brought up by the Rails input handling vulnerability a
> while back [1]. To sum that up, it seems that a common Rails idiom is
> something like, using Django syntax:
>
>  MyModel.objects.create(request.POST)
>
> (only more complicated). To avoid people having access to privileged
> fields, you are supposed to set an attribute on MyModel to restrict the
> fields that can be set in this way. This is a really poor design choice
> (which they've finally admitted [2]), since it is insecure-by-default,
> and led to the embarrassing github incident, and probably many more
> unknown vulnerabilities in Rails apps.
>
> For us, we don't have this issue because we have the forms layer that
> sits between the HTTP request and model creation, and that's the
> recommended way to do things.
>
> However, in the case of ModelForms, you can easily get to a situation
> where you've effectively got the same thing as Rails. In theory you've
> got the forms layer, but in reality your form was autogenerated using
> all the fields on the model. This happens if you use a ModelForm without
> explicitly defining Meta.fields - which is the easiest thing to do since
> it is less work.
>
> While you can use 'Meta.exclude' to remove specific fields, you still
> have to remember to do that.
>
> This is particularly dangerous in two fairly common situations:
>
> 1) You add new fields to the model that are not supposed to be publicly
> edited.
>
> 2) In the template, you are listing form fields individually, not just
> doing {{ form.as_p }}, so you can't even see the fact that other fields
> are editable, but the view/form code does allow an attacker to edit
> those fields.
>
> Also, the UpdateView CBV makes it very 

Re: QuerySet refactoring

2012-06-13 Thread Luke Plant
On 13/06/12 08:09, Anssi Kääriäinen wrote:

> Accidentally clicked send... So, what I am asking is: Is there support
> for ORM refactoring, and the "small step at time" way of doing it? If
> the ORM refactorings are to be done, it will be hard to get reviews.
> In practice I would need to commit patches without full reviews.
> 
> For more technical questions: Is changing the API of utils.tree
> acceptable? What about removing old undocumented features ("anything
> containing add_to_query() will shortcut qs.add_q() - not tested, not
> documented"). qs.order_by("tablename.columname") - tested, but not
> documented. Syntax predates 1.0). What about the more radical changes,
> like using .clone() instead of .deepcopy() in query.clone() (ticket
> #16759).
> 
> I know I have taken too much stuff under work already... However, the
> ORM is what I really like to hack, and would like to concentrate on
> that for some time. To do so I will need to feel confident that there
> is support for getting the patches committed.

I think this is a very necessary piece of work. The problem with that
layer of code is that it is very difficult to really grok and therefore
to review patches. It would take almost as much effort to do a review of
a substantial patch as the patch itself. You have to really understand
what Query is doing, and it is a huge class, that takes hours to even
vaguely get the hang of and get in your head.

I would like to be able to help, and if I dropped everything else I
might otherwise do on Django, that should be possible. I think with two
pairs of eyes on these changes, and with a good test suite, we should be
reasonably safe, but getting more review than that would be hard.

I would say that any undocumented APIs are fair game for changing, which
covers all of the Query class and SQLCompiler classes, AFAIK. For
user-facing features, we should be much more slow to break them,
especially if they are tested.  qs.order_by("tablename.columnname") is
pretty strange though, I would be happy with getting rid of that,
assuming there is an alternative.

I do actually have another proposal, that has been a growing draft email
on my computer for a few weeks: that we rewrite our backend using
SQLAlchemy Core (not the ORM). I've used SQLAlchemy a bit, and I'm very
confident we could use it profitably. While the ORM works significantly
differently to ours, the lower layer (Core - the SQL Expression
Language) can indeed build queries that can do all the joins etc that we
would need - it's a full wrapper around SQL. Some of the complexity in
our code comes from not having this wrapper.

SQLAlchemy is a really excellent library, and I fear that long term if
we don't switch to it, we will just produce an ad-hoc, informally
specified, buggy, slow implementation of half of SQLAlchemy.

However, that is a longer term aim (Django 2.0 really, for various
reasons), and I'm confident this refactoring would help either way.

Nonetheless, I'll try to post that proposal soon. There is one strategy
for implementing it that might allow it to be started soon, and
developed alongside existing functionality without breaking anything.
(That depends on when we think 'Django 2.0' will happen). If you've got
a particular interest in the ORM, you might be interested in that proposal.

I completely agree regarding how to handle NoSQL as well.

Regards,

Luke


-- 
"Outside of a dog, a book is a man's best friend... inside of a
dog, it's too dark to read."

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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Daniel Sokolowski
(I apologize if this is a duplicate - my other account appears to have an 
issue sending to this group)


I'm in the camp don't change it.  ModelForms are doing what they are 
designed to do: providing a very handy short cut. It is a design decision to 
either use a ModelForm, or a a plain Form not tied to a model. If I want 
front end user functionality I use a Form, if I want admin front end 
functionality I use a ModelForm - I use a different approach based on the 
expected level of trust here.  The un-desired effect would also only come 
into play if these new fields were not required so they would need to pass 
validation - yes?  Actually I would suggest to remove 'fields' and leave the 
'exclude' option, if I remove one two or three fields I might still have 
functioning ModelForm where I don't have to override save() to pass DB 
validation , any more excludes and a customized Form might be a better 
approach;  having the 'fields' option implies to me I can get away using 
ModelForm instead of Form and have falsely in past tried to do so.


-Original Message- 
From: Luke Plant

Sent: Tuesday, June 12, 2012 7:16 PM
To: django-developers@googlegroups.com
Subject: ModelForms and the Rails input handling vulnerability

Hi all,

On django-core we've been discussing an issue that was security related.
However, we decided it wasn't urgent enough to warrant a security
release, and so we're moving the discussion here (especially because we
couldn't agree on what to do).

For the record, I brought up the issue initially, and favour the most
secure of the options outlined below, but I'll try to summarise with as
little bias as I can!

The issue was brought up by the Rails input handling vulnerability a
while back [1]. To sum that up, it seems that a common Rails idiom is
something like, using Django syntax:

  MyModel.objects.create(**request.POST)

(only more complicated). To avoid people having access to privileged
fields, you are supposed to set an attribute on MyModel to restrict the
fields that can be set in this way. This is a really poor design choice
(which they've finally admitted [2]), since it is insecure-by-default,
and led to the embarrassing github incident, and probably many more
unknown vulnerabilities in Rails apps.

For us, we don't have this issue because we have the forms layer that
sits between the HTTP request and model creation, and that's the
recommended way to do things.

However, in the case of ModelForms, you can easily get to a situation
where you've effectively got the same thing as Rails. In theory you've
got the forms layer, but in reality your form was autogenerated using
all the fields on the model. This happens if you use a ModelForm without
explicitly defining Meta.fields - which is the easiest thing to do since
it is less work.

While you can use 'Meta.exclude' to remove specific fields, you still
have to remember to do that.

This is particularly dangerous in two fairly common situations:

1) You add new fields to the model that are not supposed to be publicly
edited.

2) In the template, you are listing form fields individually, not just
doing {{ form.as_p }}, so you can't even see the fact that other fields
are editable, but the view/form code does allow an attacker to edit
those fields.

Also, the UpdateView CBV makes it very easy to be vulnerable - it will
generate a ModelForm class with all fields editable by default.

== Options ==

There were three main courses of action that we talked about on django-core.

= Option 1: Leave things as they are, just document the problem. This
was the least popular view on django-core.

= Option 2: Deprecate Meta.exclude, but still allow a missing
Meta.fields attribute to indicate that all fields should be editable.

The policy here is essentially this: if any fields the model are
sensitive, assume all are potentially, and require whitelisting, not
blacklisting.

= Option 3: Make the 'Meta.fields' attribute a required attribute, which
must list all the model fields that should be editable. Without it,
ModelForms would not work at all.

This also means deprecating Meta.exclude (it's redundant once you are
saying that all fields should be explicitly whitelisted).


Note that the admin would not be affected under any of these options -
the admin has its own mechanism for setting the Meta.fields, and "all
fields editable by default" is a good policy for something like the admin.

For either 2) or 3), we would be making the change in Django 1.6, with a
deprecation path - faster than our normal deprecation path, but not
immediate.

Also the UpdateView and CreateView CBVs would need modifying under at
least option 3, to match the requirements of the ModelForms they will
need to generate.

== Comparison ==

== Option 1: This is the least secure, but most convenient - you have
several ways to specify which fields should be editable, you can use
whichever you like. "We're all consenting adults here".

An argument in favour of keeping 

Re: Should custom django-admin commands always explicitely close the db connection?

2012-06-13 Thread slafs
Hello

I would like to bump this thread. Recently we stroke a similar problem. Are 
there any official changes in Django regarding this topic?

Thanks in advance.

Regards


W dniu czwartek, 23 grudnia 2010 22:17:12 UTC+1 użytkownik Djoume napisał:
>
> Hi,
>
> Most django-admin commands that connect to the database are never
> closing the database connection. This can be an issue because:
>
> 1) it will clutter the logs with errors like:
>
> (when using postgresql)
>
> 2010-12-11 13:09:04 EST LOG:  could not receive data from client:
> Connection reset by peer
> 2010-12-11 13:09:04 EST LOG:  unexpected EOF on client connection
>
> (when pooling connection with pgpool)
>
> Dec 23 11:10:24 Ubuntu10 pgpool: 2010-12-23 11:10:24 LOG:   pid 23693:
> ProcessFrontendResponse: failed to read kind from frontend. frontend
> abnormally exited
>
> 2) if the command is only doing SELECT it will cause the transaction to
> be rolled back, which can be an issue if any of those SELECT are
> supposed to have side effects (for instance when using stored procedure)
>
> hose 2 issues are also described here:
>
>
> http://stackoverflow.com/questions/1303654/threaded-django-task-doesnt-automatically-handle-transactions-or-db-connections
>
>
> Examples of django-admin commands that are not closing the db connection
> are the built in "cleanup" command and the django-haystack
> "update_index" command.
>
> So far I have been updating my management command to explicitly close
> the database connection. But in this bug report:
>
> https://github.com/toastdriven/django-haystack/issues/issue/227
>
>
> The authors of django-haystack seem to think this is an issue that
> should be fixed in django itself.
>
> I would like to know what the django developers think about this issue.
> It is annoying to me mainly because it makes it harder to monitor the
> logs so maybe you guys will think this is not something worth fixing in
> django?
>
> I can think of 2 options:
>
> 1) Update the django-admin commands documentation to recommend to
> explicitly close the db connection at the end of any custom commmand and
> use TransactionTestCase if you have any test using call_command.
>
> 2) Update BaseCommand.execute() so that it will always close the
> database connection, maybe by firing a command_finished signal
>
> What do you think?
>
> -- 
> Djoume Salvetti
> Director of Development
>
> T:416.601.1999 x 249
> www.trapeze.com twitter: trapeze
> 175 Bloor St. E., South Tower, Suite 900
> Toronto, ON M4W 3R8
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To view this discussion on the web visit 
https://groups.google.com/d/msg/django-developers/-/bajEPqPiqscJ.
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Torsten Bronger
Hallöchen!

Luke Plant writes:

> [...]
>
> == Options ==
>
> There were three main courses of action that we talked about on
> django-core.
>
> [...]
>
> = Option 2: Deprecate Meta.exclude, but still allow a missing
> Meta.fields attribute to indicate that all fields should be
> editable.

For our use cases, this would be very unfortunate.  We have many
models in our app, and many fields in most of them.  We "exclude"
most foreign key relationships, and sometimes fields which the user
must not be allowed to change.  Option 2 would mean that we would
have huge name list in 80% of the ModelForms.  Moreover, they needed
to be kept up-to-date, which is very error-prone.

Granted that it's worse if security is affected, but legibility and
DRY would be reduced too drastically in my opinion by option 2.

People in a similar situation like us WILL use

fields = [f.name for f in MyModel._meta.fields if f.name not in ["blah", 
"blubb"]]

but Django should not compel its users to such things.

If you add a new field, your automatic next thought should be how
(and whether) to expose it to the user.  The other way round: If a
developer adds a sensitive field and doesn't look at the code where
the model is made editable to the user, this shows a careless
attitude a library cannot prevent anyway.

Moreover:

> The policy here is essentially this: if any fields the model are
> sensitive, assume all are potentially, and require whitelisting,
> not blacklisting.

I doubt that this is sufficiently frequently true so that it would
justify the reduce of legibility and DRY.  The sensitive fields
usually are special cases, e.g. administrative or internal.  If you
add further fields to a model, however, they are "ordinary" ones.
Well, at least this is my humble assessment and experience.
Therefore, "exclude" does a sensible job for its use case, and is a
reasonable compromise between security and comfort in my opinion.

Tschö,
Torsten.

-- 
Torsten BrongerJabber ID: torsten.bron...@jabber.rwth-aachen.de
  or http://bronger-jmp.appspot.com

-- 
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.



Re: QuerySet refactoring

2012-06-13 Thread Anssi Kääriäinen
On 13 kesä, 09:50, Anssi Kääriäinen  wrote:
> I am asking for directions about what to do about
> django.db.models.sql.query (actually sql.*). I would like to refactor
> the code in small incremental steps. However, this will bring internal
> API breakages, and will likely add some more bugs temporarily.
>
> While the ORM mostly works, it IMHO needs some polish. The reasons why
> I see ORM refactoring as needed:
>   - The code is too complex and underdocumented currently. Some
> examples are query.add_q() and compiler.fill_related_selections().
>   - Because of the above, adding new features is hard. There are some
> long standing bugs which are hard to fix. There are 407 open ORM or
> models.Model tickets, of which 247 are bugs).
>   - I believe the ORM could be made faster. The ORM currently uses 4x
> the time in Python compared to the time the DB needs to parse, plan
> and execute a query like this: Model.objects.get(pk=1).
>
> Why incremental rewrite instead of total rewrite? Total rewrite will
> likely take so much time as to never actually get done. The underlying
> structure of the ORM is good enough. There are things which would
> likely be done in different way in total rewrite, but there isn't
> anything blocker quality.
>
> Why not use SQLAlchemy or some other ORM? I am no expert of
> SQLAlchemy, but I believe it doesn't actually do the same thing as
> Django's ORM. The complexity of Django's ORM comes from the need to
> handle things like subqueries for negated multijoin lookups and
> checking when to use LEFT JOIN, when INNER JOIN. SQLAlchemy doesn't do
> that as far as I know.
>
> I have no need to make the  ORM generic enough for no-SQL databases. I
> don't believe generating a generic query.py class for no-SQL databases
> is the correct approach. 80% of the code in query.py deal with joins,
> null handling, subqueries and things like that. None of those would be
> common to no-SQL DBs. Instead, no-SQL databases need to deal with
> structured records, which isn't a problem for the SQL side of the ORM.
> The common things are lookup handling and the API. The API is already
> separate from sql.*. The lookup handling should be, too.
>
> So, what I am trying to do? Things like:
>
> https://code.djangoproject.com/ticket/16759- 
> patch:https://github.com/akaariai/django/compare/ticket_16759
>   - This is a performance improvement for query.clone() which should
> alone make the ORM around 20% faster. This is definite DDN stuff, so
> don't worry, I won't be committing this one currently.
>
> https://code.djangoproject.com/ticket/17000- patch:https://github.com/
> django/django/pull/92,https://github.com/akaariai/django/compare/refactor_utils_tree
>   - Make the utils.tree saner to use. Make query.add_q and
> where.as_sql() cleaner. Fixes a couple of bugs.
>
> https://code.djangoproject.com/ticket/16715- somewhat old patch in
> ticket.
>   - Unify and fix join promotion logic.
>
> There are more tickets somewhat ready in Trac.
>
> The above are small steps to making the ORM easier to handle. The long
> term goals at the moment are just cleanup. This cleanup should allow
> new features (conditional aggregates, custom lookups etc), but is not
> the immediate goal.

Accidentally clicked send... So, what I am asking is: Is there support
for ORM refactoring, and the "small step at time" way of doing it? If
the ORM refactorings are to be done, it will be hard to get reviews.
In practice I would need to commit patches without full reviews.

For more technical questions: Is changing the API of utils.tree
acceptable? What about removing old undocumented features ("anything
containing add_to_query() will shortcut qs.add_q() - not tested, not
documented"). qs.order_by("tablename.columname") - tested, but not
documented. Syntax predates 1.0). What about the more radical changes,
like using .clone() instead of .deepcopy() in query.clone() (ticket
#16759).

I know I have taken too much stuff under work already... However, the
ORM is what I really like to hack, and would like to concentrate on
that for some time. To do so I will need to feel confident that there
is support for getting the patches committed.

 - Anssi

-- 
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.



QuerySet refactoring

2012-06-13 Thread Anssi Kääriäinen
I am asking for directions about what to do about
django.db.models.sql.query (actually sql.*). I would like to refactor
the code in small incremental steps. However, this will bring internal
API breakages, and will likely add some more bugs temporarily.

While the ORM mostly works, it IMHO needs some polish. The reasons why
I see ORM refactoring as needed:
  - The code is too complex and underdocumented currently. Some
examples are query.add_q() and compiler.fill_related_selections().
  - Because of the above, adding new features is hard. There are some
long standing bugs which are hard to fix. There are 407 open ORM or
models.Model tickets, of which 247 are bugs).
  - I believe the ORM could be made faster. The ORM currently uses 4x
the time in Python compared to the time the DB needs to parse, plan
and execute a query like this: Model.objects.get(pk=1).

Why incremental rewrite instead of total rewrite? Total rewrite will
likely take so much time as to never actually get done. The underlying
structure of the ORM is good enough. There are things which would
likely be done in different way in total rewrite, but there isn't
anything blocker quality.

Why not use SQLAlchemy or some other ORM? I am no expert of
SQLAlchemy, but I believe it doesn't actually do the same thing as
Django's ORM. The complexity of Django's ORM comes from the need to
handle things like subqueries for negated multijoin lookups and
checking when to use LEFT JOIN, when INNER JOIN. SQLAlchemy doesn't do
that as far as I know.

I have no need to make the  ORM generic enough for no-SQL databases. I
don't believe generating a generic query.py class for no-SQL databases
is the correct approach. 80% of the code in query.py deal with joins,
null handling, subqueries and things like that. None of those would be
common to no-SQL DBs. Instead, no-SQL databases need to deal with
structured records, which isn't a problem for the SQL side of the ORM.
The common things are lookup handling and the API. The API is already
separate from sql.*. The lookup handling should be, too.

So, what I am trying to do? Things like:

https://code.djangoproject.com/ticket/16759 - patch:
https://github.com/akaariai/django/compare/ticket_16759
  - This is a performance improvement for query.clone() which should
alone make the ORM around 20% faster. This is definite DDN stuff, so
don't worry, I won't be committing this one currently.

https://code.djangoproject.com/ticket/17000 - patch:https://github.com/
django/django/pull/92, 
https://github.com/akaariai/django/compare/refactor_utils_tree
  - Make the utils.tree saner to use. Make query.add_q and
where.as_sql() cleaner. Fixes a couple of bugs.

https://code.djangoproject.com/ticket/16715 - somewhat old patch in
ticket.
  - Unify and fix join promotion logic.

There are more tickets somewhat ready in Trac.

The above are small steps to making the ORM easier to handle. The long
term goals at the moment are just cleanup. This cleanup should allow
new features (conditional aggregates, custom lookups etc), but is not
the immediate goal.

-- 
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.



Re: ModelForms and the Rails input handling vulnerability

2012-06-13 Thread Anssi Kääriäinen
On 13 kesä, 02:16, Luke Plant  wrote:
> For the record, I brought up the issue initially, and favour the most
> secure of the options outlined below, but I'll try to summarise with as
> little bias as I can!

> = Option 1: Leave things as they are, just document the problem. This
> was the least popular view on django-core.
>
> = Option 2: Deprecate Meta.exclude, but still allow a missing
> Meta.fields attribute to indicate that all fields should be editable.
>
> The policy here is essentially this: if any fields the model are
> sensitive, assume all are potentially, and require whitelisting, not
> blacklisting.
>
> = Option 3: Make the 'Meta.fields' attribute a required attribute, which
> must list all the model fields that should be editable. Without it,
> ModelForms would not work at all.

I support option 2. I will try to keep the reasoning part short. The
end of post is devoted to another idea.

Here are the reasons why I think Option 3 isn't worth the trouble:

1. Users will learn to use fields = [f.attname for f in
Permission._meta.fields]. At that point we haven't gained anything in
security, but made modelforms less convenient to use.

2. The fix doesn't actually fix the issue for all cases. You are still
allowed to use the same form in a way that renders additional fields
based on user permissions or if the user is authenticated. If you
reuse the same form in multiple views for example, or your template
contains some logic to add/remove fields dynamically.

Yes, option 3 is a bit more secure than option 2, but I don't find the
cost-benefit ratio correct. It will still not protect users from this
admittedly easy to do security mistake. So, I am -0 to option 3.



The short part is now over. I will throw in another idea I think is at
least worth presenting. The idea is that ModelForms contain attribute
contains_secure_data. Defaults to True. If True, then one must use
allowed_fields when initialising the form from request. So, you could
not do this for secure form:
MyForm(request.POST)
instead you would need to do:
MyForm(request.POST, allowed_fields=('field1', 'field2'))

Assume your model Document has three fields: title, body and creator.
If the creator edits his document, he would likely be able to change
the creator to somebody else when editing his document if the form is
defined like this using 1.4 behavior:

class DocumentForm:
   class Meta:
   model = Document

But, the developer could create this form:

class DocumentForm:
class Meta:
model = Document
fields = ('title', 'body')
contains_secure_data = False

This would would not need allowed_fields for form.__init__().

Alternatively, assume title shall not be changed on document edit.
Then the developer could use this form:

class DocumentForm:
class Meta:
model = Document
contains_secure_data = True

The developer would then use this form in create as:
DocumentForm(..., allowed_fields=('title', 'body'))
and in update as:
DocumentForm(..., allowed_fields=('body',))

My opinion is that the form.Meta isn't actually the right place to set
the security restriction. The same form can be used in different
places with different security restriction needs. The security
restriction should be done when the request data is added to the form.

The contains_secure_data could be thrown out for simplicity. However,
I do think it has its place. If allowed_fields (or form.meta.fields)
is always required, it leads to bad practices like using the "all
model.meta._fields". Ask the user if the form is security sensitive.
Only if it is, then ask for the field restrictions.

I'm not saying the above is the correct option. Just throwing in
ideas...

 - Anssi

-- 
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.